-
Notifications
You must be signed in to change notification settings - Fork 45
feat: Pretty print JSON for python. Fix missing regular expressions for masking out ephemeral information #215
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
…or masking out ephemeral information
@@ -17,7 +17,6 @@ provider: | |||
DD_TRACE_ENABLED: true | |||
DD_API_KEY: ${env:DD_API_KEY} | |||
DD_TRACE_MANAGED_SERVICES: true | |||
DD_CAPTURE_LAMBDA_PAYLOAD: true |
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 did you remove this?
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.
I inadvertently added it when I wrote the feature, but it includes all headers and payload data that is complex to mask out, so I removed it now.
I think my introduction of this is what led to the tests being in the state they're in now.
@@ -175,14 +175,15 @@ for handler_name in "${LAMBDA_HANDLERS[@]}"; do | |||
# Replace invocation-specific data like timestamps and IDs with XXXX to normalize logs across executions | |||
logs=$( | |||
echo "$raw_logs" | | |||
node parse-json.js | |
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, how were tests passing before if there were so many values that needed to be masked? |
tests/integration/serverless.yml
Outdated
@@ -17,7 +17,6 @@ provider: | |||
DD_TRACE_ENABLED: true | |||
DD_API_KEY: ${env:DD_API_KEY} | |||
DD_TRACE_MANAGED_SERVICES: true | |||
DD_CAPTURE_LAMBDA_PAYLOAD: true | |||
lambdaHashingVersion: 20201221 |
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.
Version 3 doesn't require us to use lambdaHashingVersion anymore. Should we remove it or update it? https://www.serverless.com/framework/docs/guides/upgrading-v3#lambda-hashing-algorithm
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.
Will remove, thanks!
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.
👍 LGTM
@nhinsch - RE: the tests issue. I don't know, I noticed 3.53 merge passed all integration tests, and so did the PR (https://github.com/DataDog/datadog-lambda-python/runs/5159541822?check_suite_focus=true#step:10:349) But I don't know how they would, since they failed on my machine when I tried to replicate... |
What does this PR do?
Motivation
Testing Guidelines
Additional Notes
Types of Changes
Check all that apply