-
Notifications
You must be signed in to change notification settings - Fork 513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(instrumentation-aws-sdk): sqs message id missing on send command #968
fix(instrumentation-aws-sdk): sqs message id missing on send command #968
Conversation
Codecov Report
@@ Coverage Diff @@
## main #968 +/- ##
==========================================
+ Coverage 95.91% 95.94% +0.02%
==========================================
Files 13 24 +11
Lines 856 1404 +548
Branches 178 293 +115
==========================================
+ Hits 821 1347 +526
- Misses 35 57 +22
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Did you check to verify it's working for v2 as well? I see that test-all-version failed in your setup but was it tested with at least one v2 version?
I see the unit-test is added for v3 only. Does it makes sense also adding it to v2 here?
case 'SendMessage': | ||
span.setAttribute( | ||
SemanticAttributes.MESSAGING_MESSAGE_ID, | ||
response.data.MessageId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it guaranteed that data is not nullish? I recommend using optional chaining, so we don't throw if data
is not valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. It's strange that any
is used. It would be better to refactor the code to use unknown
to avoid this kind of issues in the future.
@blumamir I'm unable to run
Might relate to issue with
Anyways, i have added the test for sdk v2. I have just copied the whole test from v3. One interesting issue is that the |
plugins/node/opentelemetry-instrumentation-aws-sdk/test/sqs.test.ts
Outdated
Show resolved
Hide resolved
… set on send command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thank you for the research and for working on a high-quality fix for that.
Added 2 nits but they are both optional - only if you have time and motivation
Which problem is this PR solving?
Short description of the changes
I am not sure about implementation for
SendMessageBatch
as there cannot be a singlemessage id
be set, as there are multiple of them.Checklist
npm run test-all-versions
for the edited package(s) on the latest commit if applicable.