-
Notifications
You must be signed in to change notification settings - Fork 45
Feat: Add ability to tag Lambda request and response payloads for 2.7… #180
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
import json | ||
import logging | ||
|
||
redactable_keys = ["authorization", "x-authorization", "password", "token"] |
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.
How did we decide on these four keys for redaction? Should we provide a way to redact other keys?
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.
We provide datadog.yaml
obfuscation rules - these were 4 common items we wanted to automatically strip out to prevent accidental ingestion of secrets before a user has time to redact for themselves
@@ -1,21 +1,23 @@ | |||
# Contributing |
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.
Thank you for cleaning this up! 🙏
@@ -181,6 +185,10 @@ def _after(self, event, context): | |||
flush_extension() | |||
|
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.
Can we enable this feature in the integration tests as well for improved test coverage?
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.
Yeah, great idea.
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.
It appears that we don't care about span tags in our integration tests, because the integration tests are unaffected by this ENV variable (off or on). We can do this during an engineering sprint, perhaps?
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.
Hmm, that's weird, the integration tests are supposed to capture span tags. Let's dig into this a bit more before merging to make sure nothing is wrong. Happy to chat on Zoom about this if you want.
Super nit: I don't think you need to specify "for 2.7 and 3.x" in the PR title -- all PRs apply to those versions 🙂 (Unless we wait until Thursday to merge this, at which time we will probably be dropping support for 2.7!) |
… and 3.x
What does this PR do?
When
DD_CAPTURE_LAMBDA_PAYLOAD
is enabled (and tracing is enabled), will automatically wrap and (deeply) tag both the incoming request and outgoing response.Motivation
Feature request
Testing Guidelines
Additional Notes
Types of Changes
Check all that apply