Fix inconsistent exception logging patterns and add guidance#8231
Conversation
699d1bd to
e01ec36
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8231 +/- ##
=========================================
Coverage 90.27% 90.27%
+ Complexity 7695 7694 -1
=========================================
Files 850 850
Lines 23213 23207 -6
Branches 2356 2356
=========================================
- Hits 20955 20951 -4
+ Misses 1531 1529 -2
Partials 727 727 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Sorry for the delayed review. I've reworked the CONTRIBUTING.md and introduced a knowledge base at https://github.com/open-telemetry/opentelemetry-java/tree/main/docs/knowledge Can you merge main and include a section about this guidance in https://github.com/open-telemetry/opentelemetry-java/blob/main/docs/knowledge/general-patterns.md? Thanks! |
e01ec36 to
86fd832
Compare
…uidance Category A — Remove redundant e.getMessage() where exception is already passed as the Throwable parameter: - GrpcExporter.java - HttpExporter.java - JaegerRemoteSampler.java (also fix stale FINEST log to pass exception as Throwable instead of concatenating) Category B — Pass exception as Throwable instead of stringifying via getMessage(), so logging frameworks can render full stack traces: - AutoConfiguredOpenTelemetrySdkBuilder.java - DeclarativeConfiguration.java - TracerShim.java - SdkDoubleHistogram.java - SdkLongHistogram.java Add best practice guidance to CONTRIBUTING.md recommending the use of dedicated logger overloads that accept a Throwable parameter. Resolves open-telemetry#8228
86fd832 to
718b3c7
Compare
Thanks for the heads up, @jack-berg ! I've rebased on main and moved the logging guidance over to the Logging section in |
…y-java into fix-exception-logging-patterns
|
@jack-berg Thanks for the approval! Could you go ahead and merge this when you get a chance? I don't have merge access on the repo. |
|
Yup - was waiting for a passing build after resolving a merge conflict |
|
Thank you for your contribution @VamshikrishnaMonagari! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
Category A : Remove redundant
e.getMessage()where exception is already passed as the Throwable parameter:GrpcExporter.javaHttpExporter.javaJaegerRemoteSampler.java(also fix stale FINEST log to pass exception as Throwable instead of concatenating)Category B : Pass exception as Throwable instead of stringifying via
getMessage(), so logging frameworks can render full stack traces:AutoConfiguredOpenTelemetrySdkBuilder.javaDeclarativeConfiguration.javaTracerShim.javaSdkDoubleHistogram.javaSdkLongHistogram.javaAdd best practice guidance to
CONTRIBUTING.mdrecommending the use of dedicated logger overloads that accept a Throwable parameter.Resolves #8228