-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Logging exceptions with context #45136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
e0d0a7d
to
524a5a7
Compare
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
API change check API changes are not detected in this pull request. |
sdk/clientcore/core/src/main/java/io/clientcore/core/http/models/ETag.java
Outdated
Show resolved
Hide resolved
sdk/clientcore/core/src/main/java/io/clientcore/core/instrumentation/logging/ClientLogger.java
Show resolved
Hide resolved
...ore/core/src/main/java/io/clientcore/core/instrumentation/logging/ExceptionLoggingEvent.java
Outdated
Show resolved
Hide resolved
...ore/core/src/main/java/io/clientcore/core/instrumentation/logging/ExceptionLoggingEvent.java
Outdated
Show resolved
Hide resolved
sdk/clientcore/core/src/main/java/io/clientcore/core/instrumentation/logging/LoggingEvent.java
Outdated
Show resolved
Hide resolved
sdk/clientcore/core/src/main/java/io/clientcore/core/instrumentation/logging/LoggingEvent.java
Show resolved
Hide resolved
sdk/clientcore/core/src/main/java/io/clientcore/core/instrumentation/logging/LoggingEvent.java
Show resolved
Hide resolved
0b51031
to
e96f0c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request updates exception logging across the codebase to log additional context using the new throwableAtError()/log() methods instead of the previous logThrowableAsError() approach. Key changes include replacing exception throwing and logging patterns with more context-rich variants and removing unnecessary exception wrapping in various client, credential, and test classes.
Reviewed Changes
Copilot reviewed 104 out of 106 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
sdk/clientcore/core/src/main/java/io/clientcore/core/http/pipeline/HttpRetryPolicy.java | Updated exception logging in retry loops and interruption handling. |
sdk/clientcore/core/src/main/java/io/clientcore/core/http/pipeline/HttpRedirectOptions.java | Simplified exception throwing using new logging methods. |
sdk/clientcore/core/src/main/java/io/clientcore/core/http/pipeline/HttpPipelineBuilder.java | Added key/value context for invalid policy positions and updated logging style. |
sdk/clientcore/core/src/main/java/io/clientcore/core/http/paging/PagedIterable.java | Changed logging when no more elements are available. |
sdk/clientcore/core/src/main/java/io/clientcore/core/http/models/Response.java, HttpRequest.java, HttpRange.java, ETag.java | Updated exception logging methods to include contextual information. |
sdk/clientcore/core/src/main/java/io/clientcore/core/http/client/JdkHttpClientBuilder.java | Refactored unsupported operation exceptions with new logging methods. |
sdk/clientcore/core/src/main/java/io/clientcore/core/credentials/* | Updated key credential and named credential validations to use the new logging pattern. |
sdk/clientcore/annotation-processor(-test)/* | Replaced wrapped RuntimeExceptions with more direct exception throwing and improved logging output. |
sdk/clientcore/annotation-processor-test/src/main/java/io/clientcore/annotation/processor/test/* | In test implementations, updated exception handling to use HttpResponseException or UnsupportedOperationException as appropriate. |
Files not reviewed (2)
- sdk/clientcore/core/checkstyle-suppressions.xml: Language not supported
- sdk/clientcore/core/spotbugs-exclude.xml: Language not supported
sdk/clientcore/core/src/main/java/io/clientcore/core/http/pipeline/HttpRetryPolicy.java
Show resolved
Hide resolved
...tation-processor/src/main/java/io/clientcore/annotation/processor/utils/ResponseHandler.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per offline discussions, it'd be good to split up T log(String, Throwable, BiFunction<String, Throwable, T>)
into a few different methods that better align with calls.
T log(String, BiFunction<String, Throwable, T>)
becomes T log(String, Function<String, T)
and T log(Throwable, BiFunction<String, Throwable, T>)
becomes T log(Throwable, Function<Throwable, T>)
, so we don't need certain BiFunction
values ignored if the constructor called by the BiFunction
doesn't support String or Throwable.
This should be fine if we add it as part of release as the compiler should warn ahead of time when a calling pattern is invalid or ambiguous.
sdk/clientcore/core/src/main/java/io/clientcore/core/http/models/Response.java
Outdated
Show resolved
Hide resolved
sdk/clientcore/core/src/main/java/io/clientcore/core/http/paging/PagedIterable.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/io/clientcore/core/http/pipeline/OAuthBearerTokenAuthenticationPolicy.java
Outdated
Show resolved
Hide resolved
c5894cf
to
17341b6
Compare
sdk/clientcore/core/src/main/java/io/clientcore/core/models/CoreException.java
Outdated
Show resolved
Hide resolved
.../core/src/main/java/io/clientcore/core/implementation/utils/AuthenticateChallengeParser.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work! Thanks for doing this.
df60a93
to
1fa740a
Compare
…ation/utils/AuthenticateChallengeParser.java Co-authored-by: Bill Wert <[email protected]>
1fa740a
to
98efc13
Compare
Fix #44865
Exception messages don't contain all the useful information we put in logs (e.g. connection-id when connection drops).
This PR allows to create and log exception using the same context in log and inside the exception message:
Results in exception message like :
java.io.IOException: connection dropped; {"connectionId":"foo","linkName":1}
and log message:
{"message":"connection dropped","exception.type":"java.io.IOException","connectionId":"foo","linkName":1}