Skip to content

Remove duplicative FINEST-level error logging in GrpcExporter#8216

Merged
jack-berg merged 1 commit intoopen-telemetry:mainfrom
VamshikrishnaMonagari:remove-duplicative-finest-otlp-error-logging
Mar 30, 2026
Merged

Remove duplicative FINEST-level error logging in GrpcExporter#8216
jack-berg merged 1 commit intoopen-telemetry:mainfrom
VamshikrishnaMonagari:remove-duplicative-finest-otlp-error-logging

Conversation

@VamshikrishnaMonagari
Copy link
Copy Markdown
Contributor

The onError method in GrpcExporter logged failures twice: once at SEVERE (with the exception object) and again at FINEST. The FINEST log is redundant since anyone who configures their logging framework to display stack traces will get them from the SEVERE log's exception parameter.

HttpExporter does not have this pattern and required no changes.

Resolves #8098

@VamshikrishnaMonagari VamshikrishnaMonagari requested a review from a team as a code owner March 27, 2026 03:58
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 27, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (9692c2b) to head (dfd0240).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...telemetry/exporter/internal/grpc/GrpcExporter.java 0.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8216      +/-   ##
============================================
- Coverage     90.33%   90.31%   -0.03%     
+ Complexity     7655     7652       -3     
============================================
  Files           843      843              
  Lines         23071    23071              
  Branches       2311     2311              
============================================
- Hits          20841    20836       -5     
- Misses         1515     1516       +1     
- Partials        715      719       +4     

☔ 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.

The FINEST log in onError used string concatenation (+ e) which only
called toString(), missing the full stack trace. Changed to pass the
exception as the Throwable parameter so the logging framework can
properly render the complete stack trace at FINEST level.

Resolves open-telemetry#8098
@VamshikrishnaMonagari VamshikrishnaMonagari force-pushed the remove-duplicative-finest-otlp-error-logging branch from 6eff0da to dfd0240 Compare March 28, 2026 20:45
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);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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:

logger.log(
Level.SEVERE,
"Failed to export "
+ type
+ "s. The request could not be executed. Full error message: "
+ e.getMessage(),
e);

logger.log(
Level.SEVERE,
"Failed to execute "
+ TYPE
+ "s. The request could not be executed. Error message: "
+ e.getMessage(),
e);

In these cases we include the stringified caught exception message and don't record the exception, preventing the user from seeing the stacktrace:

throw new ConfigurationException(
"Invalid duration property " + name + "=" + value + ". " + ex.getMessage());

logger.warning(
"Error closing " + closeable.getClass().getName() + ": " + ex.getMessage());

logger.log(
Level.WARNING,
"Exception caught while extracting span context; returning null. "
+ "Exception: [{0}] Message: [{1}]",
new String[] {e.getClass().getName(), e.getMessage()});

logger.warning(
"Error closing " + closeable.getClass().getName() + ": " + ex.getMessage());

logger.warning("Error setting explicit bucket boundaries advice: " + e.getMessage());
return this;

logger.warning("Error setting explicit bucket boundaries advice: " + e.getMessage());

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #8228 to discuss / track separately.

@jack-berg jack-berg merged commit c16394a into open-telemetry:main Mar 30, 2026
26 of 27 checks passed
@otelbot
Copy link
Copy Markdown
Contributor

otelbot bot commented Mar 30, 2026

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Do we need to log OTLP errors at SEVERE and FINEST?

3 participants