Conversation
This comment has been minimized.
This comment has been minimized.
🟢 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. |
aa21edf to
506a231
Compare
4bfc4b6 to
113957f
Compare
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
|
e7e6409 to
b7fa1e8
Compare
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
|
amarziali
changed the title
13947ba to
c034c2c
Compare
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
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. |
amarziali
added
type: feature
![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: c034c2c2b7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7dcb090 to
9549280
Compare
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9549280e8f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: beed56821b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13ee8f8522
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
🤖 Generated with Claude Code
|
🤖 Generated with Claude Code General concern separate from the inline nitpicks: "strict trace writes" isn't a hard guarantee the tracer can enforce everywhere — it depends on async completion racing against root-span close, which the tracer doesn't control for every integration. The old This PR deletes those overrides outright rather than replacing them with the new 🤖 Generated with Claude Code |
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
🤖 Generated with Claude Code
| } | ||
| } | ||
|
|
||
| private static boolean isNotOnTop(Object scope) { |
There was a problem hiding this comment.
Wrong-thread-close detection (isNotOnTop, and the scopeSourceOf/continuationOf helpers it shares the pattern with) relies entirely on cached reflective Field/Method lookups against unqualified private/package-private members (scopeManager, scopeStack(), checkTop), with every failure path swallowed by catch (Throwable ignored) { return false; } and the null/failed result cached in a static field forever.
If ContinuableScope.scopeManager, ContinuableScopeManager.scopeStack(), or ScopeStack.checkTop are ever renamed or refactored, the cached lookup permanently fails for the rest of the JVM's lifetime, so the close-wrong-thread diagnostic silently stops firing for every subsequent test with no exception, log, or test failure — defeating one of this PR's stated detection goals without any signal that it broke.
🤖 Generated with Claude Code
There was a problem hiding this comment.
The current tests check the reflective members and verify that wrong-thread events, source metadata, and continuation links are actually recorded. Failed lookups also retry rather than being cached permanently.
These are CI regression guards, not per-test startup validation—my earlier “fail-fast” wording was imprecise. I consider this sufficient for this PR and propose resolving the thread.
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
🤖 Generated with Claude Code
|
@dougqh Regarding your general concern: These are separate controls: disabling continuation tracking does not disable strict trace writes, so the annotation wouldn’t replace the removed overrides. The diagnostic helped identify why those tests previously needed strict writes disabled. We fixed the underlying instrumentation and test-lifecycle issues in separate PRs, making those overrides unnecessary rather than ignoring their original reasons. Async work can legitimately outlive root-span closure; correctly captured continuations keep the trace pending. Longer term, my intention is to remove the strict-write escape hatch from instrumentation tests entirely, so lifecycle problems are investigated rather than hidden. That’s a separate change, outside this PR. Any remaining demonstrated limitation should still be assessed and documented individually. |
This branch has not been deployed
2 participants
What Does This Do
Adds default-on scope and continuation lifecycle diagnostics to instrumentation tests.
The diagnostic records continuation and scope events during each test, waits briefly for asynchronous cleanup, and fails with an actionable timeline when it detects problems such as:
The timeline includes elapsed time, thread transitions, ownership state, and relevant call sites so the missing lifecycle edge can be identified without reconstructing it from a trace timeout.
flowchart LR A[Capture continuation] --> B[Resume context] B --> C[Run asynchronous work] C --> D[Close scope] D --> E[Release continuation] E --> F[Write complete trace] C -. missing cleanup .-> G[Test ends] G --> H[Diagnostic failure] H --> I[Timeline with thread and callsite] style E fill:#d5f5e3,stroke:#239b56 style H fill:#fadbd8,stroke:#c0392bHow is it enabled?
The diagnostic runs automatically for both instrumentation-test harnesses:
A narrow opt-out is available only for tests that are incompatible with the diagnostic itself:
@TrackScopeContinuations(
enabled = false,
reason = "Explain the incompatibility and removal condition")
The reason is mandatory. The opt-out is not intended for unresolved continuation leaks.
Additional behavior
Motivation
Strict trace writing can reveal that asynchronous work is still pending, but it does not explain where ownership was lost.
This diagnostic connects capture, activation, scope closure, and continuation resolution into one timeline, making continuation failures reproducible and actionable for both humans and automated integration work.
Fixed issues leveraging that:
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]