fix(instrumentation-aws-sdk): sqs message id missing on send command

[mentos1386]

Which problem is this PR solving?

• Fixes aws-sdk: sqs instrumentation is not setting messaging.message_id on send request #964

Short description of the changes

• Added test that makes sure that message id attribute is added to span on send command
• Fixed response hook implementation to add message id attribute to span on send command

I am not sure about implementation for SendMessageBatch as there cannot be a single message id be set, as there are multiple of them.

Checklist

• Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.
  • I ran it, but it failed due to unrelated issue.

[codecov]

Codecov Report

Merging #968 (3bc4665) into main (eba9531) will increase coverage by 0.02%.
The diff coverage is 89.47%.

@@            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     

Impacted Files	Coverage Δ
...emetry-instrumentation-aws-sdk/src/services/sqs.ts	88.05% <89.47%> (ø)
...ntation-aws-sdk/src/services/ServicesExtensions.ts	100.00% <0.00%> (ø)
...entation-aws-sdk/src/services/MessageAttributes.ts	90.32% <0.00%> (ø)
...etry-instrumentation-aws-sdk/src/services/index.ts	100.00% <0.00%> (ø)
...opentelemetry-instrumentation-aws-sdk/src/enums.ts	100.00% <0.00%> (ø)
...emetry-instrumentation-aws-sdk/src/services/sns.ts	93.75% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts	97.91% <0.00%> (ø)
...try-instrumentation-aws-sdk/src/services/lambda.ts	97.77% <0.00%> (ø)
...y-instrumentation-aws-sdk/src/services/dynamodb.ts	100.00% <0.00%> (ø)
... and 2 more

[blumamir]

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?

[blumamir]

Is it guaranteed that data is not nullish? I recommend using optional chaining, so we don't throw if data is not valid.

[mentos1386]

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.

[mentos1386]

@blumamir I'm unable to run test-all-versions, i get weird issue:

/home/tine/projects/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-aws-sdk/node_modules/mocha/node_modules/yargs/lib/usage.js:240
      .concat(Object.keys(yargs.parsed.newAliases) || [])
                     ^
TypeError: Cannot convert undefined or null to object

Might relate to issue with npm run bootstrap where i have to run npm run install --force to make it "work":

❯ npm run bootstrap

> opentelemetry-contrib@0.1.0 bootstrap
> lerna bootstrap --no-ci

lerna notice cli v3.22.1
lerna info versioning independent
lerna info Bootstrapping 48 packages
lerna info Installing external dependencies
lerna ERR! npm install exited 1 in '@opentelemetry/instrumentation-amqplib'
lerna ERR! npm install stderr:
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! While resolving: @opentelemetry/instrumentation-amqplib@0.28.0
npm ERR! Found: @opentelemetry/api@1.0.1
npm ERR! node_modules/@opentelemetry/api
npm ERR!   dev @opentelemetry/api@"1.0.1" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer @opentelemetry/api@"^1.0.2" from @opentelemetry/contrib-test-utils@0.28.0
npm ERR! node_modules/@opentelemetry/contrib-test-utils
npm ERR!   dev @opentelemetry/contrib-test-utils@"^0.28.0" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR!
npm ERR! See /home/tine/.npm/eresolve-report.txt for a full report.

---

Anyways, i have added the test for sdk v2. I have just copied the whole test from v3.

One interesting issue is that the SemanticAttributes.HTTP_STATUS_CODE is undefined on v2 but on v3 is 200. So i have to comment out that test. I haven't look deeper why it is that way.

[blumamir]

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