-
Notifications
You must be signed in to change notification settings - Fork 966
Remove duplicative FINEST-level error logging in GrpcExporter #8216
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -133,7 +133,7 @@ private void onError( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| + e.getMessage(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| e); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (logger.isLoggable(Level.FINEST)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.log(Level.FINEST, "Failed to export " + type + "s. Details follow: " + e); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.log(Level.FINEST, "Failed to export " + type + "s. Details follow:", e); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We actually have inconsistent and probably bad patterns around this in other places: In these cases we log both the exception message and record the the exception itself: Lines 133 to 134 in 9a992c2
Lines 119 to 125 in 9a992c2
Lines 130 to 136 in 9a992c2
In these cases we include the stringified caught exception message and don't record the exception, preventing the user from seeing the stacktrace: Lines 184 to 185 in 9a992c2
Lines 504 to 505 in 9a992c2
opentelemetry-java/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java Lines 110 to 114 in 9a992c2
Lines 290 to 291 in 9a992c2
opentelemetry-java/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java Lines 97 to 98 in 9a992c2
opentelemetry-java/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkLongHistogram.java Line 102 in 9a992c2
We should add some guidance to https://github.com/open-telemetry/opentelemetry-java/blob/main/CONTRIBUTING.md#best-practices-that-we-follow to recommend not stringifying the exception and recording the exception using dedicated log overloads.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened #8228 to discuss / track separately. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| result.failExceptionally(FailedExportException.grpcFailedExceptionally(e)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.