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.

Sign up for GitHub

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

Jump to bottom

Add tag_body configuration option for Shoryuken #1449

Merged
merged 2 commits into from
Apr 12, 2021
Merged

Add tag_body configuration option for Shoryuken #1449

merged 2 commits into from
Apr 12, 2021

Conversation

gkampjes
Copy link
Contributor

@gkampjes gkampjes commented Apr 4, 2021

This PR adds the option to configure tagging of Shoryuken jobs with the SQS message body.

I've made it default to false, which changes the default behaviour. However, I think this is probably the best option as:

  1. This brings the configuration in line with the other job library instrumentations (Sidekiq, Sneakers, Que etc.)
  2. It's a secure default (I think it's a reasonable expectation that the SQS message can/will contain sensitive data)

@gkampjes gkampjes requested a review from a team April 4, 2021 17:50
marcotc
marcotc previously approved these changes Apr 5, 2021
View reviewed changes
Copy link
Member

@marcotc marcotc left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @gkampjes!
Despite this being a breaking change for users of Shoryuken (the change of tag_body to default to false), I'm leaning towards accepting the default value change, as it provides better sensitive information management by default.

@marcotc marcotc self-assigned this Apr 5, 2021
@marcotc marcotc added community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations labels Apr 5, 2021
@gkampjes
Copy link
Contributor Author

gkampjes commented Apr 9, 2021

@marcotc Any idea what's causing the specs to fail? Looks like it's a regression introduced by #1453?

marcotc
marcotc approved these changes Apr 12, 2021
View reviewed changes
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @gkampjes! 🎉

@marcotc marcotc merged commit da95afc into DataDog:master Apr 12, 2021
@github-actions github-actions bot added this to the 0.48.0 milestone Apr 12, 2021
@delner delner added this to Released in Active work Apr 19, 2021
@delner delner removed this from Released in Active work Apr 19, 2021
@marcotc
Copy link
Member

marcotc commented Apr 19, 2021

👋 @gkampjes, your changes have just been released in 0.48.0 🎉

@delner delner added this to Released in Active work Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcotc marcotc marcotc approved these changes

Assignees

@marcotc marcotc

Labels
community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations
Projects
Active work
  
Released
Milestone
0.48.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gkampjes @marcotc