Skip to content
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

feat: Use base64 encoded JSON and Binary type to avoid message being … #1889

Merged
merged 3 commits into from
Mar 3, 2022

Conversation

astuyve
Copy link
Collaborator

@astuyve astuyve commented Mar 1, 2022

…dropped by filter policies

What does this PR do?

A report (DataDog/serverless-plugin-datadog#232) led us to uncover that there's an issue with passing string which contains JSON via SNS Message Attributes. Although AWS is aware of the issue, we'd rather move quickly on this and address it.

This PR moves to using a binary datatype and passing a base64 encoded string instead.

Motivation

Plugin Checklist

Additional Notes

@astuyve astuyve requested a review from a team as a code owner March 1, 2022 21:00
rochdev
rochdev previously approved these changes Mar 1, 2022
@rochdev rochdev added bug Something isn't working integrations labels Mar 1, 2022
@rochdev rochdev added this to the 2.3.1 milestone Mar 1, 2022
@rochdev rochdev merged commit 2a3bb58 into master Mar 3, 2022
@rochdev rochdev deleted the aj/sns-use-b64-and-binary branch March 3, 2022 17:49
rochdev pushed a commit that referenced this pull request Mar 3, 2022
#1889)

* feat: Use base64 encoded JSON and Binary type to avoid message being dropped by filter policies

* feat: Use BinaryValue, not StringValue

* Feat: AWS automatically base64's the data for us, so we don't want to repeat it
rochdev pushed a commit that referenced this pull request Mar 3, 2022
#1889)

* feat: Use base64 encoded JSON and Binary type to avoid message being dropped by filter policies

* feat: Use BinaryValue, not StringValue

* Feat: AWS automatically base64's the data for us, so we don't want to repeat it
@onyxraven
Copy link

onyxraven commented May 17, 2022

This PR was originally marked as 'breaking-change' - and it definitely is, when working with a consumer in a language that is statically typed (java). The change from a String/StringValue to Binary/BinaryValue breaks any assuptions consumer languages have on consuming that header (wether they mean to or not).

Specifically, the documented sqs-java-messaging-lib does not support Binary type headers at all, and throws an exception. This is obviously not a great behavior for this library in the first place, but does represent a breaking interface change here in the trace layer that I cannot escape from currently.

There are a couple ways out of this in that java library, all involving forking and adding functionality.

But I'm wondering, the approach in this library continues to use the _datadog header, when it looks like others are using the AWS Trace ID x-ray header instead for sns/sqs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants