-
Notifications
You must be signed in to change notification settings - Fork 430
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(tracer): implement AWS payload tagging for request/response #10642
Conversation
|
BenchmarksBenchmark execution time: 2024-11-12 20:45:03 Comparing candidate commit c6c5551 in PR branch Found 0 performance improvements and 16 performance regressions! Performance is the same for 372 metrics, 2 unstable metrics. scenario:sethttpmeta-all-disabled
scenario:sethttpmeta-all-enabled
scenario:sethttpmeta-collectipvariant_exists
scenario:sethttpmeta-no-collectipvariant
scenario:sethttpmeta-no-useragentvariant
scenario:sethttpmeta-obfuscation-no-query
scenario:sethttpmeta-obfuscation-regular-case-explicit-query
scenario:sethttpmeta-obfuscation-regular-case-implicit-query
scenario:sethttpmeta-obfuscation-send-querystring-disabled
scenario:sethttpmeta-obfuscation-worst-case-explicit-query
scenario:sethttpmeta-obfuscation-worst-case-implicit-query
scenario:sethttpmeta-useragentvariant_exists_1
scenario:sethttpmeta-useragentvariant_exists_2
scenario:sethttpmeta-useragentvariant_exists_3
scenario:sethttpmeta-useragentvariant_not_exists_1
scenario:sethttpmeta-useragentvariant_not_exists_2
|
8bdb59a
to
eea1069
Compare
Mapping from Java implementation
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.
Approving from an API perspective
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 🎉
Seems that environment leaks to other tests
…so they don't get flagged as misspelled during doc build
I'm slightly concerned by the new vendored libraries and by the potential performance of this solution. I'd love to see some benchmarking/profiling to get an idea of what the CPU overhead looks like, if possible 🙏 |
) ## Overview This pull request adds the ability to expand AWS request/response payloads as span tags. This matches our lambda offerings and provides useful information to developers when debugging communication between various AWS services. This is based on the AWS Payload Tagging RFC and this implementation in [dd-trace-node](DataDog/dd-trace-js#4309) and this implementation in [dd-trace-java](DataDog/dd-trace-java#7312). This feature is _disabled_ by default. When activated this will produce span tags such as: ``` "aws.request.body.PublishBatchRequestEntries.0.Id": "1", "aws.request.body.PublishBatchRequestEntries.0.Message": "ironmaiden", "aws.request.body.PublishBatchRequestEntries.1.Id": "2", "aws.request.body.PublishBatchRequestEntries.1.Message": "megadeth" "aws.response.body.HTTPStatusCode": "200", ``` ## Configuration There are five new configuration options: - `DD_TRACE_CLOUD_REQUEST_PAYLOAD_TAGGING`: - `""` by default to indicate that AWS request payload expansion is **disabled** for _requests_. - `"all"` to define that AWS request payload expansion is **enabled** for _requests_ using the default `JSONPath`s for redaction logic. - a comma-separated list of user-supplied `JSONPath`s to define that AWS request payload expansion is **enabled** for _requests_ using the default `JSONPath`s and the user-supplied `JSONPath`s for redaction logic. - `DD_TRACE_CLOUD_RESPONSE_PAYLOAD_TAGGING`: - `""` by default to indicate that AWS response payload expansion is **disabled** for _responses_. - `"all"` to define that AWS response payload expansion is **enabled** for _responses_ using the default `JSONPath`s for redaction logic. - a comma-separated list of user-supplied `JSONPath`s to define that AWS request payload expansion is **enabled** for _responses_ using the default `JSONPath`s and the user-supplied `JSONPath`s for redaction logic. - `DD_TRACE_CLOUD_PAYLOAD_TAGGING_MAX_DEPTH` (not defined in RFC but done to match NodeJS): - sets the depth after which we stop creating tags from a payload - defaults to a value of `10` - `DD_TRACE_CLOUD_PAYLOAD_TAGGING_MAX_TAGS` (to match Java implementation) - sets the maximum number of tags allowed to be expanded - defaults to a value of `758` - `DD_TRACE_CLOUD_PAYLOAD_TAGGING_SERVICES` (to match Java implementation) - a comma-separated list of supported AWS services - defaults to ` s3,sns,sqs,kinesis,eventbridge` ## Other - [`jsonpath-ng` has been vendored](https://github.com/h2non/jsonpath-ng/blob/master/jsonpath_ng/jsonpath.py) - [`ply` has been vendored (v3.11) (dependency of `jsonpath-ng`)](https://github.com/dabeaz/ply/releases/tag/3.11) ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --------- Co-authored-by: erikayasuda <[email protected]>
Overview
This pull request adds the ability to expand AWS request/response payloads as span tags.
This matches our lambda offerings and provides useful information to developers when debugging communication between various AWS services.
This is based on the AWS Payload Tagging RFC and this implementation in dd-trace-node and this implementation in dd-trace-java.
This feature is disabled by default.
When activated this will produce span tags such as:
Configuration
There are five new configuration options:
DD_TRACE_CLOUD_REQUEST_PAYLOAD_TAGGING
:""
by default to indicate that AWS request payload expansion is disabled for requests."all"
to define that AWS request payload expansion is enabled for requests using the defaultJSONPath
s for redaction logic.JSONPath
s to define that AWS request payload expansion is enabled for requests using the defaultJSONPath
s and the user-suppliedJSONPath
s for redaction logic.DD_TRACE_CLOUD_RESPONSE_PAYLOAD_TAGGING
:""
by default to indicate that AWS response payload expansion is disabled for responses."all"
to define that AWS response payload expansion is enabled for responses using the defaultJSONPath
s for redaction logic.JSONPath
s to define that AWS request payload expansion is enabled for responses using the defaultJSONPath
s and the user-suppliedJSONPath
s for redaction logic.DD_TRACE_CLOUD_PAYLOAD_TAGGING_MAX_DEPTH
(not defined in RFC but done to match NodeJS):10
DD_TRACE_CLOUD_PAYLOAD_TAGGING_MAX_TAGS
(to match Java implementation)758
DD_TRACE_CLOUD_PAYLOAD_TAGGING_SERVICES
(to match Java implementation)s3,sns,sqs,kinesis,eventbridge
Other
jsonpath-ng
has been vendoredply
has been vendored (v3.11) (dependency ofjsonpath-ng
)Checklist
Reviewer Checklist