feat(spring-cloud-aws): instrument onMessage(Collection<Message<T>>) … #19053
feat(spring-cloud-aws): instrument onMessage(Collection<Message<T>>) … #19053Aryainguz wants to merge 20 commits into
Conversation
|
|
Aryainguz
changed the title
Aryainguz
changed the title
Aryainguz
force-pushed
the
fix-aws-sqs-batch
branch
3 times, most recently
from
99088d1 to
6f62474
Compare
Aryainguz
force-pushed
the
fix-aws-sqs-batch
branch
4 times, most recently
from
72eb903 to
6c901e8
Compare
…for batch consumption
Aryainguz
force-pushed
the
fix-aws-sqs-batch
branch
from
6c901e8 to
1a51d04
Compare
| List<String> result = messageFuture.get(10, SECONDS); | ||
| assertThat(result).containsExactly(messageContent); | ||
|
|
||
| testing.waitAndAssertTraces( |
There was a problem hiding this comment.
This doesn't look right. The trace structure does not follow what other instrumentations use for batch sending and receiving. There are no receive/process spans. The callback span on the receiver side is in the same trace as the producer which is unexpected. Usually the batch scenarios create separate traces for receiver and producer that are connected via span links. I guess if the batch receive really receives only 1 message it is possible to model this in a single trace but if there are multiple messages then it won't be possible. Image that batch receive receives 2 message A and B that are part of different traces. I don't know by heart whether we have any instrumentations that would change the trace structure of batch receives depending on whether 1 ore more messages were received.
There was a problem hiding this comment.
Thanks for review, @laurit You were right.
The issue was that the code was entirely skipping the creation of the process span for batch handlers.
To fix this, I updated SpringAwsUtil to explicitly create a proper CONSUMER process span when handling batches, and updated the acknowledgement step to properly link back to that trace. The tests have been updated to verify the new trace structure and everything is passing locally. Please review again and let me know what you think
There was a problem hiding this comment.
I would expect a trace structure similar to
where producer and receiver are in separate traces.…e scope closure to include throwable status
…ect parent span assertion in tests
| } | ||
| } | ||
|
|
||
| static volatile Consumer<List<String>> batchMessageHandler; |
There was a problem hiding this comment.
I've updated the implementation to explicitly model the single-span batch behavior with correct batch semantics. The batch process span now includes the messaging.batch.message_count attribute and is linked to the producer spans of all messages in the batch. I've also added multi-message test coverage to validate this regression doesn't happen.
| if (messages.isEmpty()) { | ||
| return null; | ||
| } | ||
| Message<?> message = messages.iterator().next(); |
There was a problem hiding this comment.
AI generated comment (I have not verified it) --
[General] handleBatch() starts a process span from only messages.iterator().next(), so a real batch with multiple SQS messages only gets tracing context/process telemetry for the first message. The AWS SDK TracingList path normally traces each received SQS message as it is processed, and this instrumentation explicitly disables that iterator tracing during Spring conversion so the listener advice can do the tracing later. With the current implementation, messages after the first are effectively dropped from the process-span side of the instrumentation, and user callback work is parented only to the first message’s span. The new test sends a singleton batch, so it would not catch this regression. Please either trace each message in the collection or make the single-span batch behavior explicit with correct batch semantics and multi-message test coverage.
|
This PR has review comments. Review suggestions, whether from maintainers or automated reviewers, aren't always correct or required. Please evaluate each comment on its merits, then make sure each thread has a clear outcome. For example, link to the commit if you applied a suggestion, explain why it wasn't applied, or ask a follow-up question. Automation flags a PR for human review once every review thread has a reply or is marked as resolved. Status across open PRs is visible on the pull request dashboard. |
…lable annotation to getDestination
…mproved validation
…ent and manual warmup
…ng SQS message processing
…y of SQS batch listener tests
…BATCH_MESSAGE_COUNT attribute
Fixes #19052