-
Notifications
You must be signed in to change notification settings - Fork 37
feat: Support binary based decoding for SNS message attributes #269
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #269 +/- ##
==========================================
+ Coverage 82.75% 82.84% +0.08%
==========================================
Files 37 37
Lines 1618 1626 +8
Branches 349 351 +2
==========================================
+ Hits 1339 1347 +8
Misses 231 231
Partials 48 48
Continue to review full report at Codecov.
|
@@ -283,6 +325,42 @@ describe("readTraceFromEvent", () => { | |||
}, | |||
], | |||
}); | |||
expect(result).toEqual({ |
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.
Somehow, I forgot to add an expectation to this test 🤦🏻
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.
lol
@@ -283,6 +325,42 @@ describe("readTraceFromEvent", () => { | |||
}, | |||
], | |||
}); | |||
expect(result).toEqual({ |
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.
lol
…ction (#7205) ## Summary of changes SNS -> SQS context propagation was broken, as we encode the value of the `_datadog` header into base64. On the consume side, we assume it is written as a string, which is only true if we send a message directly through SQS. The fix is to decode the `_datadog` value based on the type, just as in [this](DataDog/datadog-lambda-js#269) PR. This soles issues where the DSM map doesn't show links between SNS and SQS, as the checkpoint is still created on the consumer, just without a link to the parent. ## Reason for change ## Implementation details ## Test coverage I tested this via system-tests, with a description of my test methodology in [this PR](DataDog/system-tests#4897). The SNS system test above pass when I ran locally with this tracer build. ## Other details <!-- Fixes #{issue} --> <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
…ction (#7205) ## Summary of changes SNS -> SQS context propagation was broken, as we encode the value of the `_datadog` header into base64. On the consume side, we assume it is written as a string, which is only true if we send a message directly through SQS. The fix is to decode the `_datadog` value based on the type, just as in [this](DataDog/datadog-lambda-js#269) PR. This soles issues where the DSM map doesn't show links between SNS and SQS, as the checkpoint is still created on the consumer, just without a link to the parent. ## Reason for change ## Implementation details ## Test coverage I tested this via system-tests, with a description of my test methodology in [this PR](DataDog/system-tests#4897). The SNS system test above pass when I ran locally with this tracer build. ## Other details <!-- Fixes #{issue} --> <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
What does this PR do?
Fixes DataDog/serverless-plugin-datadog#232
TODO: Bump linked version of dd-trace in this PR
Motivation
Although AWS documentation claims that string values are supported, AWS support has told us that stuffing JSON into a string is not supported, and breaks filter policies entirely. These messages are not rejected, and are successfully delivered to recipients without a filter policy, so they fail silently.
This change is backwards compatible, so users still relying on DD trace 2.1 can use this version.
Testing Guidelines
I created a test app that publishes to a topic and contains an SQS subscription with a filter policy. I've verified that these changes combined with my changes in dd trace js (DataDog/dd-trace-js#1889) properly propagate trace context and are not subject to filter policies:
Additional Notes
Types of Changes
Check all that apply