feat(aws-sdk): SQS receive: use span links instead of processing spans #2345
Conversation
seemk
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2345 +/- ##
==========================================
- Coverage 90.75% 90.75% -0.01%
==========================================
Files 169 169
Lines 8018 8014 -4
Branches 1632 1630 -2
==========================================
- Hits 7277 7273 -4
Misses 741 741
|
There was a problem hiding this comment.
Hi, thanks for addressing the previous issue(s) in this PR. Noting here that this is should be labelled as a Breaking Change (I see this is generally okay for minor version upgrades for a v0.x.y package).
Reading through the context to understand and cherry-picking some points here:
- (summarized here) Current implementation of SQS Receive's process spans has issues related to async and has limitations in JavaScript
- (discussed here) Latest OTel spec has removed mention of process spans for
Batch Receiving, enabling this PR to drop the process spans- older v1.20.0 spec has indicated that a receive span may have process spans, and each process span has a link to their respective producer spans
- newer v1.28.0 spec not only indicates that the receive span itself should have all the links to the producer contexts, but also removes process spans (creation of process spans are described as optional here)
Since this PR follows updates in the latest spec and avoids issues summarized here, I don’t mind this change. I also want to know if @blumamir has additional thoughts in regards to these change to drop process spans?
| ), | ||
| const messages: SQS.Message[] = response?.data?.Messages || []; | ||
|
|
||
| span.setAttribute('messaging.batch.message_count', messages.length); |
There was a problem hiding this comment.
You can now can use ATTR_MESSAGING_BATCH_MESSAGE_COUNT from https://github.com/open-telemetry/opentelemetry-js/blob/v1.27.0/semantic-conventions/src/experimental_attributes.ts#L4074
There was a problem hiding this comment.
I have seen some discussion that argues to stay with hardcoded attributes until they are stable, so maybe we can keep this as-is.
| @@ -604,4 +304,76 @@ describe('SQS', () => { | |||
| ); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('batch receive', () => { | |||
There was a problem hiding this comment.
Thanks for adding these tests. Currently these tests are for AWS SDK for JavaScript v2 which has entered maintenance mode and soon will not be supported. Do you mind if you can also write these tests for AWS SDK for JS v3?
Also you also could add assertions for messaging.batch.message_count to sqs tests in test/aws-sdk-v3.test.ts.
There was a problem hiding this comment.
Can you update this doc with your changes and to remove mention of processing spans
https://github.com/open-telemetry/opentelemetry-js-contrib/blob/instrumentation-aw[…]0/plugins/node/opentelemetry-instrumentation-aws-sdk/doc/sqs.md
Which problem is this PR solving?
Drop SQS processing spans in favor of span links as per latest specification.
Fixes #707
Short description of the changes
Create a span link for each incoming message that has a propagation context.