-
Notifications
You must be signed in to change notification settings - Fork 293
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
bugfix: AWS SQS MD5 Hash Mismatch #1572
bugfix: AWS SQS MD5 Hash Mismatch #1572
Conversation
Would it be possible to add @normj or @birojnayak as optional reviewers? |
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.
Looks good, I recommend adding more context to the PR description for other reviewers that are not as familiar with the internals of the AWS .NET SDK.
src/OpenTelemetry.Instrumentation.AWS/Implementation/AWSServiceHelper.cs
Show resolved
Hide resolved
@ppittle let's try to add a test if it's possible ? Customer has reported this as bug, so see if we can add a test. |
00b6d81
to
6ec938b
Compare
I have reworked the unit tests to reflect the change in implementation. I also looked at adding a proper regression test - https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1572/files#diff-e5d83ef47b6e8c2bac87983d6e1ede500c9c42506c262afb183d4ff7bb557db2R177 However, this doesn't fully cover the issue raised. The exception is caused because the older version didn't send all Attributes to the SQS Service. When SQS replied with the hash of what it had received, this did not match the hash calculated by the SDK and an exception was raised by the user. The unit tests intercept the SDK Pipeline to Set a Mock Web Response rather than making a live call to a SQS instance. Covering this issue in a unit test would require the test to precalculate the expected hash and inject that value into the mock response. In my opinion, the maintenance cost for that test code is high and would not provide a high degree of certainty of being able to detect future changes to the SDK Pipeline's internals. It would be more appropriate to have an integration test, but without an AWS Account to test against, this is also not a feasible option. I recommend we leave the test coverage as it is, but I welcome a second opinion |
6ec938b
to
31344ee
Compare
@@ -198,8 +199,8 @@ private void ValidateAWSActivity(Activity aws_activity, Activity parent) | |||
|
|||
private void ValidateDynamoActivityTags(Activity ddb_activity) | |||
{ | |||
Assert.Equal("DynamoDBv2.Scan", ddb_activity.DisplayName); | |||
Assert.Equal("DynamoDBv2", Utils.GetTagValue(ddb_activity, "aws.service")); | |||
Assert.Equal("DynamoDB.Scan", ddb_activity.DisplayName); |
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.
why this assert changed ?
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.
The AWSServiceName tag is set based on the result from calling AWSServiceHelper.GetAWSServiceName
. The implementation of GetAWServiceName
was changed in this PR to use ServiceMetaData.ServiceId
instead of requestContext.Request.ServiceName
; moving the handler code earlier in the SDK Pipeline means the requestContext.Request
object isn't yet available.
ServiceId
and ServiceName
are sometimes subtly different, as is the case with dynamo: DynamoDB vs DynamoDBv2.
I could see an argument that this is a breaking change - existing users would see the new tag value after upgrading. To prevent it, I'd need to hardcode a mapping table for dynamodb and sns. When I considered this, my opinion was the name change was acceptable and the maintenance overhead of the mapping table was not worth it.
But I'd very much welcome additional opinions, especially if CNCF has guidance around this issue.
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.
@Kielek or anyone , could you add tag to mark this change as breaking change ? or let us know the procedure... |
Typically, you should add entry to the Changelog. * **Breaking Change**: Your message here
([#PR_ID](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/PR_ID)) |
31344ee
to
87085d1
Compare
Updated Changelog |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
@Kielek - looks like this PR has a pending reviewer:
Are you able to approve, or point me towards someone who can? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1572 +/- ##
==========================================
+ Coverage 73.91% 80.64% +6.73%
==========================================
Files 267 114 -153
Lines 9615 3080 -6535
==========================================
- Hits 7107 2484 -4623
+ Misses 2508 596 -1912
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@ppittle Could you please fix the CI errors? We can then merge the PR. |
61c8b0e
to
5b860ad
Compare
…arlier in the pipeline. This is necessary to support sqs becoming a json service and no longer supporting writing to MessageAttributes after Marshalling has occured.
…tes after RequestContext.Request has been built so that the Propagator can inject request headers.
5b860ad
to
867ad33
Compare
Fixes #1492
Changes
A recent change to the AWS SDK implementation of SQS client changes wireline protocol from XML to Json. This causes the SDK's internal request pipeline to behave differently, causing
AWSTracingPipelineHandler
to no longer be able to manipulateSendMessageRequest.MessageAttributes
after the Marshalling step has been completed.This change moves
AWSTracingPipelineHandler
to run before Marshalling (ie Serialization) so that Open Telemetry headers can be correctly injected.Injecting propagation context into outgoing requests have been moved to a new handler,
AWSPropagatorPipelineHandler
, which runs later in the SDK request pipeline.Service Name Mapping
Moving
AWSTracingPipelineHandler
ahead in the AWS SDK Pipeline made it necessary to also tweakAWSServiceHelper
to userequestContext.ServiceMetaData.ServiceId
instead ofrequestContext.Request.ServiceName
as theRequest
has not yet been populated at this point.ServiceId and ServiceName can differ for some AWS Services, requiring an update to
AWSServiceType
. I have manually verified all 3 Service Names inAWSServiceType
for correctness.