fix: change exception logging when running in maven #7336

brendenehlers · 2025-05-09T03:04:17Z

Fix for: #6827

Added try/catch around call to OpenTelemetrySdk.close() to catch NoClassDefFoundException. If the code is running in a maven environment (determined by presence of maven.home system property), then the exception is logged out as a warning. Otherwise, the exception is re-thrown.

codecov · 2025-05-09T03:07:58Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.12%. Comparing base (1e763b2) to head (85718a0).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #7336   +/-   ##
=========================================
  Coverage     90.12%   90.12%           
- Complexity     7187     7188    +1     
=========================================
  Files           814      814           
  Lines         21700    21705    +5     
  Branches       2123     2123           
=========================================
+ Hits          19557    19562    +5     
  Misses         1477     1477           
  Partials        666      666

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

trask

thanks!

have you tested this with the repro in #6827 (comment)?

brendenehlers · 2025-05-09T23:02:51Z

@trask originally built it there, but did a sanity check with the final product and realized that the logging dependencies might not be on the classpath when that hook is called resulting in the same error originally seen but with a different class.

while it might not be the best solution, i opted for System.out since it's guaranteed to be there when the code is run, so at least the end user will get the feedback even tho it won't be as tightly tied into the logging ecosystem. i figured it's at the end of the lifecycle so it should be fine.

brendenehlers · 2025-05-10T20:43:47Z

@trask idk what changed with the test but the codecov check isn't picking up the coverage for the provided test. the code it's saying isn't covered must be hit otherwise the test would fail, so i'm wondering if i've misconfigured something from the original successful pipeline?

trask · 2025-05-14T14:57:55Z

.../src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java

+            // https://github.com/open-telemetry/opentelemetry-java/issues/6827
+            if (IS_MAVEN) {
+              // logging deps might not be on the classpath at this point
+              System.out.printf("%s Flush failed during shutdown: %s%n", Level.WARNING, e);


I think the goal here is not to (overly) alarm people, but also let them know the flush on shutdown did not occur

@cyrille-leclerc wdyt?

Suggested change

System.out.printf("%s Flush failed during shutdown: %s%n", Level.WARNING, e);

System.out.println("%s Flush failed during shutdown, which is a known issue when running OpenTelemetry inside maven (for details see https://github.com/open-telemetry/opentelemetry-java/issues/6827)");

I'm sorry, I forgot why this problem could occur in Maven builds.

The Maven OTel extension disables the OTel SDK shutdown hook in favor of explicitly shutting down the OTel SDK as part of the Maven plugin lifecycle. Is it cause by some tests like JUnit tests that run in process of the Maven build and launch an OTel SDK? Or is it happening when the Maven process is started with the OTel auto instrumentation agent?

As @trask highlighted, if there is a NoClassDefFoundError then some spans including the root spns are likely to not be exported which is a critical problem.

By the way, if this problem happens with Maven, shouldn't it also happen with the OTel Gradle Plugin?
FYI The Gradle OTel plugin also implements an explicit shutdown of the otel SDK here and there.

@cyrille-leclerc it seems to have something to do with maven's class loader, you can find a simple repro here that doesn't even instantiate OpenTelemetry SDK: #6827 (comment)

My guess would be that this isn't maven specific. Probably by the time the shutdown hook runs the class loader that contains the sdk classes has been shut down and won't be able to load any more classes. For example the URLClassLoader provides a close method that could be used to trigger this. Perhaps the easiest way to repro would be to run on jdk8 (app loader is URLClassLoader) and manually call close on the app loader. Tomcat class loader also has similar behavior. With maven exec:java the class loader is shut down in https://github.com/mojohaus/exec-maven-plugin/blob/master/src/main/java/org/codehaus/mojo/exec/ExecJavaMojo.java#L334 and the class loader is a URLClassLoader https://github.com/mojohaus/exec-maven-plugin/blob/42bc369e89e51b474b25f03475b7c40af314a4a2/src/main/java/org/codehaus/mojo/exec/URLClassLoaderBuilder.java#L119
I probably would drop all the maven specific checks and just catch the NoClassDefFoundError. There should be a comment explaining under what circumstances the NoClassDefFoundError occurs. Sometimes issues like this can be fixed by ensuring that all classes needed for shutdown are loaded before the shutdown hook is run. Here I don't think this is really viable.

So just to check my understanding, instead of handling it specifically for Maven I should change it to just catch the NoClassDefFoundError and log out the above message that @trask suggested?

brendenehlers · 2025-08-22T03:03:23Z

woah time flies... removed the maven-specific code from the class so it handles the NoClassDefFoundError with a sysout.

jkwatson · 2025-08-26T23:22:20Z

.../src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java

+          } catch (NoClassDefFoundError e) {
+            // https://github.com/open-telemetry/opentelemetry-java/issues/6827
+            // logging deps might not be on the classpath at this point
+            System.out.printf("%s Flush failed during shutdown: %s%n", Level.WARNING, e);


So, although this might all be true in one very narrow case, it seems very non-obvious that a) flush is what would have failed (since close() was called, and it might do more than that), and are we sure that NoClassDefFoundError is the only thing we want to catch here? This feels like a bandaid, and if we want this band-aid, we should make it more comprehensive, and not assume the one narrow case we've run into so far.

What did you have in mind in this case? Happy to change it whatever it needs to be!

Maybe just catch Exception or Throwable to make sure we capture any possible failures?

github-actions · 2025-09-22T23:25:59Z

This has been automatically marked as stale because it has been marked as needing author feedback and has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

brendenehlers · 2025-09-24T16:19:48Z

Updated the catch to Throwable, so it will catch anything thrown as part of the shutdown process. Figured Throwable was better since that'll also catch the Error instances, not just Exception instances.

jkwatson

Thanks!

otelbot · 2025-09-25T19:27:21Z

Thank you for your contribution @brendenehlers! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

)

change exception logging when running in maven

b00bf82

brendenehlers requested a review from a team as a code owner May 9, 2025 03:04

brendenehlers mentioned this pull request May 9, 2025

Race condition in SDK shutdown #6827

Closed

add test

6ae5cf9

trask reviewed May 9, 2025

View reviewed changes

use system.out instead of logger

6f63548

brendenehlers added 2 commits May 9, 2025 17:12

update error message

a1ba77f

add issue documentation

bd57cf1

trask reviewed May 14, 2025

View reviewed changes

brendenehlers added 2 commits August 21, 2025 20:50

remove maven-specific code

6fa723b

clean up remaining code

051b0eb

brendenehlers changed the title ~~change exception logging when running in maven~~ fix: change exception logging when running in maven Aug 22, 2025

remove unused import

92f7649

brendenehlers requested a review from trask August 22, 2025 03:25

jkwatson reviewed Aug 26, 2025

View reviewed changes

jkwatson added the needs author feedback Waiting for additional feedback from the author label Sep 11, 2025

github-actions bot added Stale and removed Stale labels Sep 22, 2025

brendenehlers added 2 commits September 24, 2025 08:34

Merge branch 'main' into exception-fix

d6859b4

update to throwable

85718a0

github-actions bot removed the needs author feedback Waiting for additional feedback from the author label Sep 24, 2025

brendenehlers requested a review from jkwatson September 24, 2025 22:47

jkwatson approved these changes Sep 25, 2025

View reviewed changes

jkwatson merged commit 7ea2dd9 into open-telemetry:main Sep 25, 2025
29 checks passed

the-clam pushed a commit to the-clam/opentelemetry-java that referenced this pull request Nov 6, 2025

fix: change exception logging when running in maven (open-telemetry#7336

0e4b2b6

)

	System.out.printf("%s Flush failed during shutdown: %s%n", Level.WARNING, e);
	System.out.println("%s Flush failed during shutdown, which is a known issue when running OpenTelemetry inside maven (for details see https://github.com/open-telemetry/opentelemetry-java/issues/6827)");

fix: change exception logging when running in maven #7336

fix: change exception logging when running in maven #7336

Uh oh!

Conversation

brendenehlers commented May 9, 2025

Uh oh!

codecov bot commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

trask left a comment

Choose a reason for hiding this comment

Uh oh!

brendenehlers commented May 9, 2025

Uh oh!

brendenehlers commented May 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendenehlers commented Aug 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 22, 2025

Uh oh!

brendenehlers commented Sep 24, 2025

Uh oh!

jkwatson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

otelbot bot commented Sep 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented May 9, 2025 •

edited

Loading