Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds stable messaging v1.43 process-duration and consumed-message metrics to AWS Lambda SQS instrumentation while preserving existing span behavior.
Changes:
- Registers process metrics for batch and per-message processing.
- Records consumed-message counts once per batch.
- Adds success, failure, default-mode, and topology-aware metric assertions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
AwsLambdaSqsInstrumenterFactory.java |
Registers stable SQS operation metrics. |
AwsLambdaSqsMessageHandlerTest.java |
Tests nested and failed processing metrics. |
AwsLambdaSqsEventWrapperTest.java |
Tests wrapper-generated metrics. |
AwsLambdaSqsEventHandlerTest.java (3.11) |
Supplies the library instrumentation name. |
AwsLambdaSqsMetricsAssertions.java |
Adds shared metric assertions. |
AbstractAwsLambdaSqsEventHandlerTest.java |
Verifies batch event metrics. |
AwsLambdaSqsEventHandlerTest.java (javaagent) |
Supplies the agent instrumentation name. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
`hasPointsSatisfying` delegates to AssertJ's `satisfiesExactlyInAnyOrder`, which already requires the points to match the given assertions and no more. The separate `satisfies(data -> assertThat(...getPoints()).hasSize(1))` call therefore asserts nothing that the following `hasPointsSatisfying(...)` does not already assert. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
trask
force-pushed
the
trask-add-lambda-sqs-messaging-metrics
branch
from
August 14, 2026 00:26
050d81f to
7091dea
Compare
Review finding: The new assertMetrics method carries @SuppressWarnings("deprecation") with the comment "using deprecated semconv", but nothing it references is deprecated. Verified against opentelemetry-semconv-incubating 1.43.0-alpha with javap: MessagingIncubatingAttributes.MESSAGING_SYSTEM, MESSAGING_DESTINATION_NAME and MESSAGING_OPERATION_NAME carry no Deprecated attribute, MessagingSystemIncubatingValues and its AWS_SQS constant carry none either, and ErrorAttributes.ERROR_TYPE is stable. The deprecated MESSAGING_OPERATION key, which is what justifies the same suppression on AbstractAwsLambdaSqsEventHandlerTest and on AwsLambdaSqsMessageHandlerTest.processSpans, is never used in this file. The suppression is therefore inert today and its comment is factually wrong, and it will silently swallow any genuine deprecation warning introduced in this method later. Fix: delete the @SuppressWarnings("deprecation") annotation and its comment from assertMetrics. Analysis: AwsLambdaSqsMetricsAssertions only references non-deprecated symbols. Every semconv key it uses (MESSAGING_SYSTEM, MESSAGING_DESTINATION_NAME, MESSAGING_OPERATION_NAME, MessagingSystemIncubatingValues.AWS_SQS) and the stable ErrorAttributes.ERROR_TYPE carry no Deprecated attribute in the resolved semconv artifacts, and neither do the enclosing classes or the assertion and metric APIs the method calls. The deprecated MESSAGING_OPERATION key that justifies the identical suppression in the sibling span-assertion tests is never used here, so the annotation was inert from the moment the file was added. Upsides: Removes a suppression whose comment misstates what the code does, and restores deprecation warnings for this method so a future migration to a deprecated API is surfaced rather than silently swallowed. Downsides: No material downside identified; the module still compiles cleanly without the suppression. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
trask
marked this pull request as ready for review
August 14, 2026 04:01
Pull request dashboard statusMerged · refreshed 2026-08-14 17:24 UTC Status above doesn't look right?
|
laurit
approved these changes
Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the v1.43 messaging metrics to AWS Lambda SQS processing, so Lambda functions consuming SQS events now report
messaging.process.durationandmessaging.client.consumed.messagesalongside their existing process spans.The batch event instrumenter records both metrics, counting every SQS record delivered in the event exactly once. The nested per-message instrumenter records only
messaging.process.duration, so consumed messages are not double counted. A processing exception that propagates out addserror.typeto both.Nothing changes on the default path: the new instruments are only created when the stable messaging semantic conventions are enabled, and no spans are affected either way.
Stacked on #19478. Part of #19254.