Add grpc circuit breaker utility using interceptors

[pavan-traceable]

Description

Added grpc utility to support circuit breaker for slow and failure calls

Testing

Tested changes on sandbox environment using this utility.

[github-actions]

Test Results

90 tests  +7   90 ✅ +7   24s ⏱️ +2s
16 suites +2    0 💤 ±0 
16 files   +2    0 ❌ ±0 

Results for commit f163a9d. ± Comparison against base commit 20402cf.

♻️ This comment has been updated with latest results.

[codecov]

Codecov Report

Attention: Patch coverage is 33.63229% with 148 lines in your changes missing coverage. Please review.

Project coverage is 60.52%. Comparing base (20402cf) to head (f163a9d).

Files with missing lines	Patch %	Lines
...s/resilience/ResilienceCircuitBreakerProvider.java	2.22%	44 Missing ⚠️
...tbreaker/grpcutils/CircuitBreakerConfigParser.java	0.00%	43 Missing ⚠️
...ence/ResilienceCircuitBreakerRegistryProvider.java	5.55%	17 Missing ⚠️
...ls/resilience/ResilienceCircuitBreakerFactory.java	0.00%	11 Missing ⚠️
...esilience/ResilienceCircuitBreakerInterceptor.java	81.03%	7 Missing and 4 partials ⚠️
...breaker/grpcutils/CircuitBreakerConfiguration.java	0.00%	9 Missing ⚠️
...ience/ResilienceCircuitBreakerConfigConverter.java	66.66%	6 Missing and 1 partial ⚠️
...itbreaker/grpcutils/CircuitBreakerInterceptor.java	25.00%	3 Missing ⚠️
...uitbreaker/grpcutils/CircuitBreakerThresholds.java	78.57%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##               main      #68       +/-   ##
=============================================
- Coverage     71.80%   60.52%   -11.28%     
- Complexity      173      193       +20     
=============================================
  Files            26       35        +9     
  Lines           532      755      +223     
  Branches         26       47       +21     
=============================================
+ Hits            382      457       +75     
- Misses          126      268      +142     
- Partials         24       30        +6     

Flag	Coverage Δ
unit	60.52% <33.63%> (-11.28%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[aaron-steinfeld]

We discussed supporting the key function returning null too. And if a default exists,I think the logic should look something like

key = null
if (keyFunction not null) {
  key = keyFunction(context, message)
}
circuitBreaker =  key not null ? getCircuitBreaker(key) : getDefaultCircuitBreaker();

if (circuitBreaker is null) {
  // short circuit
}

// continue with existing logic

[pavan-traceable]

We are setting default always why will circuitBreaker become null ?
builder.defaultThresholds(
config.hasPath(DEFAULT_THRESHOLDS) ?
buildCircuitBreakerThresholds(config.getConfig(DEFAULT_THRESHOLDS)) :
buildCircuitBreakerDefaultThresholds());

[pavan-traceable]

If someone defines keyFunction, why key should be null ? To handle what scenarios ? Can you please list all scenarios that we need to handle, i will check accordingly

[aaron-steinfeld]

We are setting default always why will circuitBreaker become null ?

We're requiring config for it, but it can be disabled and would not have an associated circuit breaker.

If someone defines keyFunction, why key should be null ?

Imagine an API like "doAction(action)" where we want to circuit break some expensive action like "email" by tenant ID. A key function might look like action == "email" ? getTenantId() : null. That would allow other actions to continue on without a circuit breaker, but emails to be circuit broken by tenant ID. Alternatively, we could add a separate optional filter method with a contract like (context, request) => boolean. Do you think that would be clearer?

I realize looking back my logic was a bit off here too.

Can you please list all scenarios that we need to handle, i will check accordingly

Basically any permutation - should be able to provide a key function (and filter/predicate, if we want to separate that - the class match is a bit of an implicit predicate but does not encompass all scenarios) which allows us to:

1. route some requests to custom circuit breaker that's declared in config
2. route others to a default circuit breaker that's declared in config
3. have others skip the circuit breaker entirely

[pavan-traceable]

Hopefully i have fixed all different scenarios now. Please check

[aaron-steinfeld]

Why are we writing this into the map? The the default thresholds don't need to be accessed by key - that should be what builder.defaultThresholds is for. Also note if DEFAULT_THRESHOLDS path does exist it would make it into the map, but it would not be set to builder.defaultThresholds. I would expect this logic to look more like

builder.defaultThresholds(
  config.hasPath(DEFAULT_THRESHOLDS) ?
    buildCircuitBreakerThresholds(config.getConfig(DEFAULT_THRESHOLDS)) : 
    buildCircuitBreakerDefaultThresholds());

The config key then is not a concern of the specific impl. If you really want to use a magic key, then defaultThresholds would be removed, we wouldn't want both since it's two ways of conveying the same thing - one just decouples concerns.

[pavan-traceable]

done

[pavan-traceable]

This is done mainly to handle where circuit breaker is disabled by default but enabled for only tenant, in CircuitBreakerProvider we check enabled or based on CircuitConfigMap only, so thats why adding to map.

[aaron-steinfeld]

So these default thresholds go into the registry directly, but the others are built as-needed?

[pavan-traceable]

We need default config for CircuitBreakerRegistry, we will override this at circuitBreaker level

[aaron-steinfeld]

So if this is some random key (that would map to the default) how do we handle there being no default here? It looks like the disabled key is only being respected for the overrides

[pavan-traceable]

Fixed it.

[aaron-steinfeld]

So I see one part of the confusion here. You're using this special key for the "shared" circuit breaker that's used when there is no key. We can use a special key for that, but it should be local to this provider - it's indepdendent of the config.

I think part of the confusion is that the term default is being overloaded a bit here. There's two separate things:

• the config to use if no key matches (which can be disabled - if no specific key config, don't circuit break)
• the shared circuit breaker to use if there is no key (which would be an instance of the former case - the config to use if no key matches).

[pavan-traceable]

Thanks for your patience, i hope i have addressed all the cases now. Due to multiple scenarios, i feel implementation is slightly complicated, not sure if we use these different scenarios.

[aaron-steinfeld]

Because NON_THRESHOLD_KEYS is not excluding the default, the default will be put into the map like every other config in addition to being set as defaultThresholds.

Can avoid putting it in the map any only use it through defaultThresholds? Because also note that if I choose to build the config in code (rather than parsing it from config) I would only set it in the one place.

[aaron-steinfeld]

This works because the defaults are still being put in the map along with every other config - but we can't rely on that as I pointed out in an earlier comment (it wouldn't hold if I built my config rather than parsed it, and it's that same abstraction leak we've been trying to avoid).

[aaron-steinfeld]

nit - can this just be return getCircuitBreaker(SHARED_KEY);?

[aaron-steinfeld]

Nit - given that most of this PR (including this same function) is working in optionals, please stick to optionals over null unless the third party code doesn't give us an option. This function can return an optional to convey it like

private Optional<CircuitBreaker> buildNewCircuitBreaker(String circuitBreakerKey, boolean defaultEnabled) {
  return Optional.ofNullable(circuitBreakerConfigMap.get(circuitBreakerKey))
        .map(config -> circuitBreakerRegistry.circuitBreaker(circuitBreakerKey, config))
        .or(() -> defaultEnabled ? Optional.of(circuitBreakerRegistry.circuitBreaker(circuitBreakerKey)) : Optional.empty());

[aaron-steinfeld]

Why not store nulls? No need to recompute the disabled status for the same key. Could make the map value an optional to avoid the null behavior of computeIfAbsent.

[aaron-steinfeld]

Are there any concerns about this growing too large? If we were to use a bad key (e.g. request ID) as our circuit breaker it would grow unbounded. Consider if we should use a true simple cache here like below to prevent a bad config from crashing a service.

        CacheBuilder.newBuilder()
            .expireAfterAccess(Duration.ofHours(1)) // or from config
            .maximumSize(10_000)
            .build(CacheLoader.from(this::buildCircuitBreaker));