fix(aws-sdk): avoid repeating MessageAttributeNames in sqs receiveMessage

[JamieDanielson]

Which problem is this PR solving?

If duplicate requests are storing the same value multiple times, this will address the problem by passing the MessageAttributeNames through a Set to de-duplicate them.

It's unclear why these duplicates are happening and adding the same value multiple times, so that is still an issue to be looked into. It is not solved by this PR. This PR is also not attempting to measure the size of the attributes to determine whether the size is under the required 256KB.

• closes SQS ReceiveMessages crashes with 413 REQUEST ENTITY TOO LARGE #1038

Short description of the changes

• Use a set to ensure unique values when concatenating propagation fields in the AWS SQS instrumentation.

Checklist

• Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

[codecov]

Codecov Report

Merging #1044 (f3e4e41) into main (23d98b1) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1044      +/-   ##
==========================================
+ Coverage   95.91%   95.95%   +0.04%     
==========================================
  Files          13       24      +11     
  Lines         856     1410     +554     
  Branches      178      295     +117     
==========================================
+ Hits          821     1353     +532     
- Misses         35       57      +22     

Impacted Files	Coverage Δ
...entation-aws-sdk/src/services/MessageAttributes.ts	91.89% <100.00%> (ø)
...emetry-instrumentation-aws-sdk/src/services/sqs.ts	87.87% <100.00%> (ø)
...opentelemetry-instrumentation-aws-sdk/src/enums.ts	100.00% <0.00%> (ø)
...etry-instrumentation-aws-sdk/src/services/index.ts	100.00% <0.00%> (ø)
...ntation-aws-sdk/src/services/ServicesExtensions.ts	100.00% <0.00%> (ø)
...try-instrumentation-aws-sdk/src/services/lambda.ts	97.77% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts	97.95% <0.00%> (ø)
...entelemetry-instrumentation-aws-sdk/src/aws-sdk.ts	97.44% <0.00%> (ø)
... and 3 more

[blumamir]

Thanks for the fix
Can you please add a test for this case?

[JamieDanielson]

Thanks for the fix Can you please add a test for this case?

We're having some trouble figuring out how to test MessageAttributeNames, and part of that may be a misunderstanding in how it works.

Our understanding at the moment is that MessageAttributeNames is set on ReceiveMessage to indicate the specific message attribute names to receive (and thus filter other message attributes). However, we cannot find a place in the code that is actually filtering the attribute names that are omitted from this.

In sqs.test.ts, we tried setting the MessageAttributeNames on .receiveMessage, but still see the message attributes coming through in the response that should be filtered out. We can tell when logging out MessageAttributeNames that we are de-duping, but are unclear where to access this in the response so we can test it.

To help us move forward, we'd love some help answering the following questions:

1. Since we can't access MessageAttributeNames in the response, what would be a good test to see if MessageAttributeNames are being de-duped correctly?
2. Where does MessageAttributeNames get used to filter the message attributes?

[blumamir]

Thank you for doing the research

Our understanding at the moment is that MessageAttributeNames is set on ReceiveMessage to indicate the specific message attribute names to receive (and thus filter other message attributes).

If I understand SQS correctly, this filtering is done in the server itself, so the request contains which message attributes to include in the response and then the server copies them to "receiveMessages" response.

However, we cannot find a place in the code that is actually filtering the attribute names that are omitted from this.

The tests for this instrumentation use a mock to return responses and validate the request parameters. This is done, for example, here. I think a proper test might be to assert that the request that is "sent" to the mock server does not repeat the propagators fields in MessageAttributeNames.

In sqs.test.ts, we tried setting the MessageAttributeNames on .receiveMessage, but still see the message attributes coming through in the response that should be filtered out. We can tell when logging out MessageAttributeNames that we are de-duping, but are unclear where to access this in the response so we can test it.

I don't know how an actual SQS server is implemented to behave if multiple names are sent in the request, it could repeat them in response or just return each one of them once. The original issue if I understood it correctly is due to the request itself being invalid due to the size (These attribute names are just keep piling up forever until the limit is hit). The response in the test is mocked and will not follow the semantics of AWS.

To help us move forward, we'd love some help answering the following questions:

1. Since we can't access MessageAttributeNames in the response, what would be a good test to see if MessageAttributeNames are being de-duped correctly?
2. Where does MessageAttributeNames get used to filter the message attributes?

I can think of 2 possible ways to test it:

1. Add assert on the "request" as it arrives to the mock server. If you do that, please make sure that a failed "expect" in the callback will catch correctly by mocha. I can see it is already "expecting" in the mock, but we had bugs like this in the past so might be worth making sure.
2. Extract this new functionality into a function in ./src/services/MessageAttributes.ts. Then you can just invoke the function directly with any interesting input and expect the result to contain each attribute name only once. There are already similar tests here. We can add a new "describe" for a function like addPropagationMessageAttributeNames(origMessageAttributeNames: string[]) which will implement this logic:

return Array.from(
            new Set([
              ...(origMessageAttributeNames ?? []),
              ...propagation.fields(),
            ])
          );

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: JamieDanielson / name: Jamie Danielson (b0fed52, 80e50df, b747f84)
• ✅ login: MikeGoldsmith / name: Mike Goldsmith (7155304)
• ✅ login: pkanal / name: Purvi Kanal (d7db9a3)

[blumamir]

Thank you for adding the test
Added 2 suggestions

[marshally]

I just wanted to leave a quick note indicating that we have tested this patch and I can confirm it resolves the issue described in #1038

Thank you @JamieDanielson, we will look forward to this getting this merged and released.

[blumamir]

LGTM, thanks for addressing all comments 💯
Left a small optional nit about the function name

[blumamir]

@JamieDanielson @pkanal , I think this is ready to be merged.
I can't merge while "This branch is out-of-date with the base branch", so please rebase this PR or "allow edits from maintainers", so we can rebase before merging.

Thanks again for working on this fix.