Return empty method lines instead of throwing when class bytecode resource is missing - #12380
Return empty method lines instead of throwing when class bytecode resource is missing#12380dougqh wants to merge 1 commit into
Conversation
…ource is missing Utils.getClassStream() is documented @nullable and can return null for classes whose .class resource can't be located on the classpath (e.g. Mockito-generated proxy classes). ByteCodeLinesResolver.ClassMethodLines.parse passed that possibly-null stream straight into ClassReader, which threw an IOException that got wrapped and logged at ERROR, then swallowed. This has been recurring in error tracking for CI Visibility test runs across many tracer versions. Short-circuit on a null stream and log at debug instead, matching the existing handling in Utils.getFileName for the same condition. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dougqh
added
tag: ai generated
Test Environment - sbt-scalatestJob Status: 🟢 success
Baseline: median of |
![datadog-prod-us1-5[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The null guard returns empty line data when the class resource is absent. The Java test migration keeps the prior cases and adds the missing-resource case.
🤖 Datadog Autotest · Commit c34874c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c34874c328
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (classStream == null) { | ||
| log.debug("Could not get input stream for class {}", clazz.getName()); | ||
| return classMethodLines; |
There was a problem hiding this comment.
When a custom class loader temporarily returns null but can provide the resource later, returning this non-null empty value from ClassMethodLines.parse causes methodLinesCache.computeIfAbsent to cache the failed lookup. Subsequent calls that hit the entry therefore never retry and continue returning missing method borders, unlike the existing exception path explicitly marked “do not cache failure”; handle the null case without storing a negative result while still suppressing the error-level log.
Useful? React with 👍 / 👎.
| ByteCodeLinesResolver linesResolver = new ByteCodeLinesResolver(); | ||
| Lines methodLines = linesResolver.getMethodLines(unresolvableMethod); | ||
|
|
||
| assertFalse(methodLines.isValid()); |
There was a problem hiding this comment.
This assertion also passes against the previous implementation: ClassReader throws for the null stream, but getMethodLines catches that exception and already returns Lines.EMPTY. Consequently, removing the new guard and restoring the production ERROR log would leave this regression test green; call ClassMethodLines.parse directly and assert it does not throw, or capture the logger and verify the error-level event is absent.
Useful? React with 👍 / 👎.
|
🎯 Code Coverage (details) 🔗 Commit SHA: c34874c | Docs | View more details | Give us feedback! |
Test Environment - nebula-release-pluginJob Status: 🟢 success
Baseline: median of |
Test Environment - netflix-zuulJob Status: 🟢 success
Baseline: median of |
Test Environment - reactive-streams-jvmJob Status: 🟢 success
Baseline: median of |
Test Environment - heliboardJob Status: 🟢 success
Baseline: median of |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Test Environment - sonar-kotlinJob Status: 🟢 success
Baseline: median of |
Test Environment - jolokiaJob Status: 🟢 success
Baseline: median of |
Test Environment - okhttpJob Status: 🟢 success
Baseline: median of |
Test Environment - spring_bootJob Status: 🟢 success
Baseline: median of |
Test Environment - sonar-javaJob Status: 🟢 success
Baseline: median of |
| /** | ||
| * A {@link ClassLoader} that defines classes from an in-memory map and returns {@code null} from | ||
| * resource lookups (rather than throwing), used to exercise {@code ByteCodeLinesResolver} when a | ||
| * class's bytecode resource cannot be located. | ||
| * | ||
| * <p>Kept in Java rather than the Java 8 test suite's Groovy counterpart on purpose: see {@link | ||
| * MisbehavingClassLoader} for why. | ||
| */ |
There was a problem hiding this comment.
nit:
| /** | |
| * A {@link ClassLoader} that defines classes from an in-memory map and returns {@code null} from | |
| * resource lookups (rather than throwing), used to exercise {@code ByteCodeLinesResolver} when a | |
| * class's bytecode resource cannot be located. | |
| * | |
| * <p>Kept in Java rather than the Java 8 test suite's Groovy counterpart on purpose: see {@link | |
| * MisbehavingClassLoader} for why. | |
| */ | |
| /** | |
| * A {@link ClassLoader} that defines classes from an in-memory map and returns {@code null} from | |
| * resource lookups (rather than throwing), used to exercise {@code ByteCodeLinesResolver} when a | |
| * class's bytecode resource cannot be located. | |
| */ |
2 participants
What Does This Do
Guards
ByteCodeLinesResolver.ClassMethodLines.parseagainst anullbytecode stream: whenUtils.getClassStream(clazz)can't locate a class's.classresource (documented@Nullable), it now returns an emptyClassMethodLinesand logs atdebug, instead of lettingClassReaderthrow anIOExceptionthat gets wrapped, logged atERROR, and swallowed.Also migrates
ByteCodeLinesResolverTestfrom Groovy/Spock to JUnit 5/Java (project convention for new/touched unit tests) and adds a regression test (NullResourceClassLoader) exercising the null-stream path directly.Motivation
Investigated a recurring, non-crashing Datadog error-tracking issue for CI Visibility (TestNG) test runs:
Could not determine method borders for {}logged at ERROR, observed across tracer versions from 1.48.1 through 1.65.1, recurring every few months whenever a test suite exercises a class whose bytecode resource isn't resolvable this way (e.g. a Mockito-generated proxy class). No functional impact (the exception was already caught and empty lines returned), but it's noisy in production logs/error tracking and keeps re-triggering an alert.Additional Notes
Utils.getFileName, which already checks for anullstream and logs atdebug../gradlew :dd-java-agent:agent-ci-visibility:test(full module) andspotlessCheck.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueagent-ci-visibilityownershipJira ticket: none (found via Datadog error tracking, not a filed ticket)