Add RxJava 3 instrumentation#11849
Conversation
🟡 Java Benchmark SLOs — Performance SLO warning (near threshold)
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. |
PerfectSlayer
left a comment
There was a problem hiding this comment.
Reviewed structure and tests. Left a bunch of comments.
| // NOTE: This test lives in the `testdog` package (not `datadog`) on purpose: the agent ignores | ||
| // `datadog.*` classes for instrumentation, so `@Trace`-annotated methods declared under `datadog.*` | ||
| // would never be instrumented. See RxJava3Test for the same convention. |
There was a problem hiding this comment.
🧹 chore: Please clean up internal comments and LLM reasoning
There was a problem hiding this comment.
Cleared, thanks
| SORT_BY_START_TIME, | ||
| span().root().operationName("interop-parent").resourceName("interop-parent"), | ||
| span() | ||
| .childOf(Worker.parentId) |
There was a problem hiding this comment.
🎯 suggestion: There is a childOfIndex() that would simplify the test case, cleaning up parentId, activeSpan().getSpanId(), etc... This is applicable to multiple parts in the test files.
There was a problem hiding this comment.
Oh, I didn't know that - looks like childOfIndex() method was introduced quite recently
Thanks
| import org.junit.jupiter.params.provider.EnumSource; | ||
|
|
||
| @WithConfig(key = "trace.otel.enabled", value = "true") | ||
| @WithConfig(key = "integration.opentelemetry-annotations-1.20.enabled", value = "true") |
There was a problem hiding this comment.
🧹 chore: This is a duplicate of trace.otel.enabled
There was a problem hiding this comment.
Duplication removed
| String method = "traceAsync" + type.type; | ||
| assertTraces( | ||
| trace( | ||
| otelSpan("RxJava3TracedMethods." + method) |
There was a problem hiding this comment.
👏 praise: Interesting use of factory method
| // NOTE: This test lives in the `testdog` package (not `datadog`) on purpose: the agent ignores | ||
| // `datadog.*` classes for instrumentation, so `@Trace`-annotated methods declared under `datadog.*` | ||
| // would never be instrumented. See the java-lang-21 tests for the same convention. |
| /** | ||
| * RxJava 3's AbstractDirectTask creates FINISHED/DISPOSED sentinel FutureTask instances in its | ||
| * static initializer. If that initializer runs while a trace is active (e.g. the first scheduled | ||
| * delay/timeout under a span), the executor instrumentation captures a continuation on those | ||
| * static singletons that is never cancelled, leaking the pending trace. Disable async propagation | ||
| * while the type initializer runs. | ||
| */ |
There was a problem hiding this comment.
🧹 chore: We don't want LLM reasoning here, here is a trimmed version to keep relevant info only:
| /** | |
| * RxJava 3's AbstractDirectTask creates FINISHED/DISPOSED sentinel FutureTask instances in its | |
| * static initializer. If that initializer runs while a trace is active (e.g. the first scheduled | |
| * delay/timeout under a span), the executor instrumentation captures a continuation on those | |
| * static singletons that is never cancelled, leaking the pending trace. Disable async propagation | |
| * while the type initializer runs. | |
| */ | |
| /** | |
| * RxJava 3's AbstractDirectTask creates FINISHED/DISPOSED sentinel FutureTask instances in its | |
| * static initializer. | |
| */ |
There was a problem hiding this comment.
Java docs was cleared and simplified
| @Advice.OnMethodExit(suppress = Throwable.class) | ||
| public static void onConstruct(@Advice.This final Maybe<?> maybe) { | ||
| Context parentContext = Java8BytecodeBridge.getCurrentContext(); | ||
| if (parentContext != null && parentContext != Java8BytecodeBridge.getRootContext()) { |
There was a problem hiding this comment.
🧹 chore: parentContext will never be null.
There was a problem hiding this comment.
Good catch, fixed
| @Advice.OnMethodExit(suppress = Throwable.class) | ||
| public static void onConstruct(@Advice.This final Observable<?> observable) { | ||
| Context parentContext = Java8BytecodeBridge.getCurrentContext(); | ||
| if (parentContext != null && parentContext != Java8BytecodeBridge.getRootContext()) { |
There was a problem hiding this comment.
🧹 chore: parentContext will never be null.
There was a problem hiding this comment.
Good catch, fixed
| @Override | ||
| public Map<String, String> contextStore() { | ||
| final Map<String, String> store = new HashMap<>(); | ||
| store.put("io.reactivex.rxjava3.core.Flowable", Context.class.getName()); |
There was a problem hiding this comment.
🎯 suggestion: Use temporary constant for context class name?
There was a problem hiding this comment.
Agree, this way much better
| @Advice.OnMethodExit(suppress = Throwable.class) | ||
| public static void onConstruct(@Advice.This final Single<?> single) { | ||
| Context parentContext = Java8BytecodeBridge.getCurrentContext(); | ||
| if (parentContext != null && parentContext != Java8BytecodeBridge.getRootContext()) { |
There was a problem hiding this comment.
🧹 chore: parentContext will never be null.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…izer RxJava 3 introduced io.reactivex.rxjava3.internal.schedulers.AbstractDirectTask, whose static initializer constructs FINISHED/DISPOSED sentinel FutureTask instances. When that initializer first runs under an active trace (e.g. the first delay/timeout scheduler hop inside a span), the executor instrumentation captures a ScopeContinuation on those static singletons that is never cancelled, leaking the pending trace so it is never reported. Disable async propagation while AbstractDirectTask's type initializer runs, mirroring the existing reactor.core.scheduler.SchedulerTask/WorkerTask handling. The matcher is inert unless RxJava 3 is on the classpath. RxJava 2 has no equivalent class. Restores the delayed-Maybe coverage in RxJava3Test, which fails without this fix and passes with it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ava3 Two tests were under datadog.trace.instrumentation.rxjava3 while the @Trace-using tests must live under testdog.* (the agent ignores datadog.* for instrumentation). Move all four tests to testdog.* for consistency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…servable and Single
…a3ResultExtensionTest
25a0260 to
a6cb915
Compare
PerfectSlayer
left a comment
There was a problem hiding this comment.
Thanks for the follow up changes!
This comment has been minimized.
This comment has been minimized.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
The merge request has been interrupted because the build 64026140340904028 took longer than expected. The current limit for the base branch 'master' is 120 minutes. Possible reasons:
|
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
bb6b14a
into
master
What Does This Do
We already trace RxJava 1 and 2 but not 3. RxJava 3 moved its types to the
io.reactivex.rxjava3.core.*namespace, so the existing rxjava-2.0 instrumentation never matches it. This adds a newrxjava-3.0module that brings RxJava 3 to parity.What's in it:
dd-java-agent/instrumentation/rxjava/rxjava-3.0module. It ports the rxjava-2.0 logic to the RxJava 3 namespace: context capture on the five reactive types (Flowable, Observable, Single, Maybe, Completable) and re-attachment around subscriber callbacks, plus the async result extension that finishes@WithSpan/@Tracespans when the stream completes, errors, or is cancelled.passfor rxjava3[3.0.0,)and afailblock that asserts the advice can never match the rxjava2 artifact, so the two versions stay isolated. The module also pulls in rxjava-2.0 as a test runtime dependency to confirm both instrumenters coexist.@WithSpanresult extension across all five types, and a Java 8 interop check (fromCompletionStage,fromStream,fromOptional). latestDepTest runs the same suite against the newest published rxjava3.One thing reviewers should look at closely: this also touches the shared
java-concurrentmodule. While testing delayed chains I found that RxJava 3 addedio.reactivex.rxjava3.internal.schedulers.AbstractDirectTask, whose static initializer builds two sentinelFutureTaskinstances. When that initializer first runs under an active trace (the firstdelayortimeouthop inside a span), the executor instrumentation captures a continuation on those static singletons that never gets cancelled, so the trace stays pending and is never reported.RxJava 2 has no equivalent class, which is why the byte for byte equivalent rxjava-2.0 code never hit this(correction: 2.0.9+ seem to have it). The fix disables async propagation while that type initializer runs, following the same pattern already used for Reactor'sSchedulerTaskandWorkerTask. The matcher is an exact class name, so it stays inert unless RxJava 3 is on the classpath.The Java 8 interop check found no propagation gaps once that fix was in place.
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]