Add custom stacktrace renderer which is length limit aware #7281
Conversation
sdk/common/src/test/java/io/opentelemetry/sdk/internal/StackTraceRendererTest.java
Show resolved
Hide resolved
| assertThat(new StackTraceRenderer(throwable, 1000).render()) | ||
| .isEqualTo(jdkStackTrace(throwable, 1000)); | ||
| assertThat(new StackTraceRenderer(throwable, Integer.MAX_VALUE).render()) | ||
| .isEqualTo(jdkStackTrace(throwable, Integer.MAX_VALUE)); |
There was a problem hiding this comment.
I love this test: for a list of exceptions render each at a variety of length limits, and compare the result to the built-in JDK stack trace renderer.
| return value; | ||
| } | ||
|
|
||
| public static void addExceptionAttributes( |
There was a problem hiding this comment.
One thing I've considered is retaining some sort of system property / env var to fallback to the built-in JDK exception rendering. Would offer a nice escape hatch to revert to the built-in jdk rendering in case there's any bugs in the code, but downside is it would be hard to know when it would be safe to finally remove.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7281 +/- ##
============================================
- Coverage 89.77% 89.75% -0.03%
- Complexity 6979 7002 +23
============================================
Files 797 798 +1
Lines 21165 21240 +75
Branches 2056 2071 +15
============================================
+ Hits 19001 19063 +62
- Misses 1503 1509 +6
- Partials 661 668 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…y-java into better-stacktrace-render
sdk/common/src/jmh/java/io/opentelemetry/sdk/internal/StacktraceRenderBenchmark.java
Show resolved
Hide resolved
sdk/common/src/main/java/io/opentelemetry/sdk/internal/StackTraceRenderer.java
Outdated
Show resolved
Hide resolved
| private static final DefaultExceptionAttributeResolver INSTANCE = | ||
| new DefaultExceptionAttributeResolver(); | ||
| new DefaultExceptionAttributeResolver( | ||
| Boolean.parseBoolean(ConfigUtil.getString(ENABLE_JVM_STACKTRACE_PROPERTY, "false"))); |
There was a problem hiding this comment.
Added new config to allow toggling between the naive JVM rendering logic and the new limits aware stacktrace rendering logic.
Given the solid testing in StackTraceRendererTest, I think we should default to the new limits aware rendering logic.
|
@open-telemetry/java-approvers please take a look - I'd like to get this in for the 7/11/25 release. |
|
I think the lack of coverage is a bit of a false positive. The key to hitting those uncovered bits to truncate exception stacktraces in all sorts of odd places. This test does that by introducing a random length limit as a parameter of the test. I think the coverage just didn't happen to randomly choose a limit which exercises those bits, but with enough iterations it should. |
3 participants
Replace
Throwable#printStackTrace(PrintStream)exception stacktrace rendering with a custom implementation which is aware of attribute length limits, and exits early to avoid unnecessary work. The result is significantly improved memory and cpu, with gains proportional to the difference between the total stack trace length and the attribute length limit. I.e. if you have a really long stack trace (i.e. 100k+ chars) and relatively small length limit (i.e. 1k chars), you'll have more to gain with this improvement.Benchmark results: