Conversation
🟢 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. |
Kafka / producer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
Kafka / consumer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
7dcb090 to
9549280
Compare
8314d04 to
7cfcfcd
Compare
94df599 to
8411dfd
Compare
7cfcfcd to
d8d4453
Compare
amarziali
added
type: feature
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-prod-us1-5[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
When a suite-setup fixture (@BeforeAll or setupSpec) fails before any test executes, the pending suite-setup diagnostic window is never flushed, so continuation failures are misattributed to suite cleanup or silently dropped — affecting both the JUnit (ScopeDiagnosticsExtension.java) and Spock (InstrumentationSpecification.groovy) instrumentation paths.
🤖 Bits Code Review · Commit d8d4453 · @DataDog review to ask questions
There was a problem hiding this comment.
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
The general direction looks good to me, but I'd like to separate the checks some.
I think some checks we should always strictly enforce in automatic instrumentation - for instance, no double finish, no double resolve, etc.
However, in manual instrumentation, I think we should tolerate misuse to avoid harming the customer application.
And since you can also use manual code to modify an automatic span, the mix needs to be tolerated, too. (That might need to be a separate PR).
I also think there are cases in automatic instrumentation where "leaks" must be tolerated.
While I understand that we want to avoid them from a timeliness and memory pressure perspective, we don’t control the APIs so we cannot guarantee that a continuation is resolved or cancelled.
So I'd like to see that check (and maybe only that check) be optional in automatic instrumentation tests. I'd still like a reason for opting out, but only from the continuation resolution/cancellation requirement.
@dougqh manual instrumentation is usually excluded by this harness since we only enforce instrumentation tests and not all the tests. Also, the annotation allows disabling for specific tests as escape hatch
I think indeed this check must be enabled and optionally disabled if and only if the enforcement is not compatible with the testing but not using it as "I cannot solve the issue in this instrumentation" since this maps to Flaky. Please also note that the enforcement already runs on master. That PR is about doing the same on BeforeAll, AfterAll (aka test once initialisers / cleanup) |
ygree
left a comment
ygree
left a comment
There was a problem hiding this comment.
The new Spock extension rejected method-level @TrackScopeContinuations annotations, breaking supported per-feature opt-outs and preventing the entire spec from running. Fixed in #12707 by accepting feature annotations, with a regression test covering the failure.
|
I still think the right solution is to split the strict checks that we can always guarantee from those that we cannot. The thing that we cannot guarantee is that a continuation will be resolved or cancelled. Unfortunately even if there is a cancel-like operation on the library being instrumented, we cannot guarantee that user code will call it. I actually think those are tests that we should be writing for each instrumentation, and I don't regard them as flaky. Maybe we just need some other more tolerant waiting mechanism for those cases instead. And while I agree it would be nice if we could make all the instrumentations fully strict all the time, I don't see how that's possible. And as long as scenarios remain where strictness cannot be guaranteed, we need a way to test it. |
Good catch and thanks for having proposed a solution @ygree I've just merged in this branch |
I agree with the narrow premise that we cannot force arbitrary application code to call a library’s cancellation API. Where I disagree is the conclusion that this means unresolved continuations must generally be tolerated by automatic instrumentation tests. The harness is not asserting that every possible application behaves correctly. It is asserting that the bounded scenario created by a test leaves no captured work behind. If a test deliberately models abandonment, then that particular test may need different expectations, but I think we should first have a concrete example and define the intended production behavior. In many cases, “the user may never call cancel” is precisely why the instrumentation must either avoid capturing the request context or release it on the library’s actual discard, timeout, close, or eviction path. |
There was a problem hiding this comment.
I agree with the intent and the implementation mostluy.
Usual test scenario should leave no unresolved continuations, that's fine by me. I guess I would have this question on shared fixtures, does it makes sense to have continuation(s) remaining valid until suite teardown? I don't have a concrete fixture requiring this today, so I'm comfortable revisiting it when one appears.
I'm approving, but before merging, please handle the nested case.
This branch has not been deployed
4 participants
What Does This Do
Extends the default continuation-leak diagnostics to suite-level test fixtures:
setupSpec()andcleanupSpec()@BeforeAlland@AfterAllThis closes the diagnostic window around work created before the first test or cleaned up after the last test. Reports identify whether a failure belongs to suite setup, an individual test, or suite cleanup.
Class-level
@TrackScopeContinuationsconfiguration applies to suite fixtures. The investigation skill has also been updated to describe the expanded coverage.flowchart LR A["Harness initializes tracer"] --> B["Suite setup<br/>setupSpec / @BeforeAll"] B --> C["Per-test diagnostics"] C --> D["Suite cleanup<br/>cleanupSpec / @AfterAll"] D --> E["Harness closes tracer"] B -. "report suite setup leaks" .-> F["Diagnostic report"] C -. "report test leaks" .-> F D -. "report suite cleanup leaks" .-> FMotivation
Continuation work created in suite fixtures previously fell outside the per-test diagnostic window. This could hide lifecycle problems caused during shared fixture initialization or shutdown.
See #12458
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 issueJira ticket: [PROJ-IDENT]