[PECOBLR-1191] Fix broken circuit breaker #1079
Conversation
samikshya-db
changed the title
| if (connectionContext == null | ||
| || telemetryDetails == null | ||
| || logLevel.toInt() < connectionContext.getTelemetryLogLevel().toInt()) { | ||
| || logLevel.toInt() > connectionContext.getTelemetryLogLevel().toInt()) { |
| * Exception indicating telemetry export failures. This intentionally does NOT emit telemetry on | ||
| * construction to avoid recursive logging loops. | ||
| */ | ||
| public class DatabricksTelemetryException extends RuntimeException { |
There was a problem hiding this comment.
why is this unchecked exception?
There was a problem hiding this comment.
Any random exception should be caught and thrown so that circuit breaker catches it.
| assertEquals(CircuitBreaker.State.CLOSED, circuitBreakerClient.getCircuitBreakerState()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
is it possible to add integration tests? So, that we can also verify this in end to end scenario
There was a problem hiding this comment.
These would require response mocking (similar to the issue we are facing with retry tests) - we can take it up later if required. I can create a ticket for now.
There was a problem hiding this comment.
Yes, create a tracking ticket
| LOGGER.trace( | ||
| "Failed to push telemetry logs with error response: {}", response.getStatusLine()); | ||
| return; | ||
| if (connectionContext.isTelemetryCircuitBreakerEnabled()) { |
There was a problem hiding this comment.
can we change log level for not able to push telemetry? Trace may be too low. We are anyways not blocking critical flow, but at least logging level can be higher than trace or debug
There was a problem hiding this comment.
We initially had error & debug, customer started complaining about verbose logs from telemetry : we changed it to trace : ideally we want to have telemetry as silent as possible. Let me know if that makes sense though.
| // 2. When telemetry details are not set. | ||
| // 3. When the telemetry logLevel configured in the connection context is higher than the | ||
| // logLevel for the event. | ||
| // 3. When the event's log level is more verbose than the configured telemetry log level. |
There was a problem hiding this comment.
Is this correct 😅? usually more log levels have lower number in frameworks. For example, log4j
event log level: DEBUG (10000)
connectionContext.getTelemetryLogLevel().toInt(): INFO (20000)
As per comment, since event log level is more verbose, we want to return. But the check 10000 > 20000 won't be true so no return.
There was a problem hiding this comment.
the numbering in this codebase is opposite to log4j's convention Actually it is the same convention, in case of telemetry, trace has a debug of 5 and info has 4 !😅
Even in log4j : it is opposite to what you have cited.
3 participants
Description
Testing
Additional Notes to the Reviewer