-
Notifications
You must be signed in to change notification settings - Fork 835
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
Incompatibility between @opentelemetry/instrumentation-http ^0.35.1 and aws-sdk v2 #3922
Comments
This was also reported before on StackOverflow https://stackoverflow.com/questions/64693442/wrong-aws-request-signature-caused-by-opentelemetry-https-plugin |
Looks like someone suggested this: https://github.com/NativeChat/otel-aws-sdk-retry-patch/tree/main |
@weyert I saw that lib, I would not suggest using that. There are multiple problems:
Maybe it'd be better to take hints from that lib and not directly use it. |
I also opened an issue on the aws-sdk-js repo. They ignore the x-amz-trace-id from the signature signing so it wouldn't be uncalled for to ignore traceparent and others either |
Hey, getting your bump on this issue would be great @leonardofreitass - aws/aws-sdk-js#4472 I have done some digging and essentially the aws-sdk-js caches the signature and doesn't recompute it when a request retries. This leads to a mismatch between the signature and the new traceparent header. The js v2 SDK is the only SDK with this caching behaviour as far as I know (I only checked botocore and js v3) The fix on the AWS end is easy but not ideal for lambda users as they would need to start packaging the aws-sdk in their lambdas to get the update. Would a work around in the instrumentation-http where we check the request is the AWS SDK V2 and skip injecting the propagation be acceptable here? |
I think it's better to not have custom logic AWS in the 'vanilla' |
To fix this there needs to be some kind of deny list for propagation as described in #1698 This wouldn't give good defaults to people using opentelemetry outside of vendors who are configuring it for people |
This looks like a duplicate of open-telemetry/opentelemetry-js-contrib#1609. There is a workaround by using the following: |
Closed based on #3922 (comment) Feel free to reopen or open a new issue if that is not correct |
What happened?
Steps to Reproduce
When using @opentelemetry/instrumentation-http version 0.35.1 or higher, whenever aws-sdk v2.x.x retries a request (e.g.: rate limit or timeout happens), the request fails, because aws-sdk adds the
traceparent
to the SignedHeaders, and as a result AWS throws a InvalidSignatureException 400 back.Expected Result
Traceparent is not added to SignedHeaders making possible to retry a failed request.
Actual Result
Traceparent is added to SignedHeaders resulting in a HTTP 400 InvalidSignatureException.
Additional Details
This should not be an issue in aws-sdk v3. Also, strange enough, this only happens on request retries. If the request is successful at first attempt, this is not a problem, making this issue even harder to reproduce and debug.
I know this is hard to fix from this package, and a proper fix would probably be aws-sdk to ignore the traceparent header, but this was definitely working prior to version 0.35.1. I managed to consistently reproduce this locally and pinpoint the offender commit to this one: 4978743
For some reason, removing the header normalization breaks request retry on aws-sdk v2. I tried to dig deeper into why this is the case, but I only managed to confirm that this piece of code being removed is definitely the one causing the issue, not why.
If anything, we might want to add somewhere that version 0.35.0 is the latest one where you can safely use with aws-sdk v2, as this is a very hard issue to reproduce and chase down.
OpenTelemetry Setup Code
package.json
Relevant log output
No response
The text was updated successfully, but these errors were encountered: