Repository navigation
Conversation
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. |
There was a problem hiding this comment.
💡 Codex Review
When the async consumer uses commitAsync() without a callback, Kafka delegates with a null callback and returns without calling enqueueUserCallbackInvocation; commitSync likewise completes through its commit future without invoking this method (Apache Kafka source). Because this instrumentation matches only enqueueUserCallbackInvocation, neither of these common successful commit paths emits a backlog update. Instrument a common successful commit-completion path rather than only user callback registration.
ℹ️ 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".
|
|
||
| // new - this instrumentation is completely new. | ||
| // the purpose of this class is to provide us with information on consumer group and cluster ID | ||
| @AutoService(InstrumenterModule.class) |
There was a problem hiding this comment.
Skip backlog updates for failed async commits
When commitAsync(..., callback) completes exceptionally, Kafka 3.8 still enqueues the callback with the requested offsets and a non-null exception (Apache Kafka source). Registering this instrumentation activates DDOffsetCommitCallback, which ignores that exception and reports every requested offset as committed, under-reporting backlog precisely when a rebalance, timeout, or broker error prevented the commit. Preserve callback delivery, but only call trackBacklog when the completion exception is null.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
yes this is known - complete overhaul is not the scope here
| new DDOffsetCommitCallback( | ||
| callback, | ||
| kafkaConsumerInfo, | ||
| InstrumentationContext.get(Metadata.class, MetadataState.class)); |
There was a problem hiding this comment.
Track backlog before invoking the application callback
When an application callback throws or mutates the supplied offsets map, DDOffsetCommitCallback invokes it before reading that map, so a successful commit can produce no backlog update or an update based on application-modified data. Because this change makes that wrapper live for every async callback, perform the Datadog bookkeeping before invoking the application callback, or otherwise isolate it with finally and an unmodified offset snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is also known. It does not look offensive - up to concerned team to open a follow up
There was a problem hiding this comment.
The newly registered callback wrapper reports requested offsets even when an asynchronous commit fails, allowing rejected commits to understate consumer lag.
🤖 Bits Code Review · Commit 58d9872
|
|
||
| // new - this instrumentation is completely new. | ||
| // the purpose of this class is to provide us with information on consumer group and cluster ID | ||
| @AutoService(InstrumenterModule.class) |
There was a problem hiding this comment.
Skip backlog updates for failed async commits
When an asynchronous commit fails, Kafka delivers the requested offsets with a non-null exception. Registering this instrumentation activates DDOffsetCommitCallback, which forwards the application callback but then reports those offsets through trackBacklog regardless of the exception. Rejected commits can therefore advance the reported committed position and understate consumer lag. Preserve callback delivery, but skip backlog reporting when the commit failed.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
🟢 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
|
What Does This Do
The async consumer’s commit callback instrumentation was missing
@AutoService, so it was not registered and did not emit DSM commit backlog metrics.This change registers the existing instrumentation and declares its required helpers and context stores. It also passes the metadata context store from advice into the callback wrapper, because InstrumentationContext.get() must be resolved in advice rather than called from a helper. The callback-based tracking design remains unchanged.
Tests verify callback delivery during poll, synchronous commit, and close. The null-group and missing-consumer-info cases now assert both callback behavior and emitted backlog metrics.
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 issueJira ticket: [PROJ-IDENT]