Adding weblog tests for TRACE_CLIENT_IP_ENABLED Configuration #3657
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
khanayan123
reviewed
Dec 10, 2024
| weblog_env={"DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING": "false"}, | ||
| weblog_env={ | ||
| "DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING": "false", | ||
| "DD_TRACE_CLIENT_IP_ENABLED": "false", |
There was a problem hiding this comment.
default behavior is false, so no need to add this for the Test_Config_ClientIPHeaderEnabled_False test
mhlidd
requested review from
gnufede,
vitor-de-araujo,
manuel-alvarez-alvarez,
jandro996 and
dmehala
and removed request for
a team
khanayan123
reviewed
Dec 11, 2024
tests/test_config_consistency.py
Outdated
| @scenarios.tracing_config_nondefault_3 | ||
| @features.tracing_configuration_consistency | ||
| class Test_Config_ClientIPHeaderEnabled_False: | ||
| """Verify headers containing ips are not tagged when DD_TRACE_CLIENT_IP_ENABLED=false |
There was a problem hiding this comment.
We should update the comment to state DD_TRACE_CLIENT_IP_ENABLED=false is default behavior. So Verify headers containing ips are not tagged by default
robertomonteromiguel
requested changes
Dec 12, 2024
The .NET parametric scenarios are unrelated (seem to be caused by this PR). I don't believe the python weblog is related either...? |
cbeauchesne
requested changes
Dec 13, 2024
tests/test_config_consistency.py
Outdated
| ) | ||
|
|
||
| def test_ip_headers_sent_in_one_request(self): | ||
| trace = [span for _, _, span in interfaces.library.get_spans(self.req, full_trace=True)] |
There was a problem hiding this comment.
The array is a lit if spans if I'm not wrong
Suggested change
| trace = [span for _, _, span in interfaces.library.get_spans(self.req, full_trace=True)] | |
| spans = [span for _, _, span in interfaces.library.get_spans(self.req, full_trace=True)] |
Suggested change
| trace = [span for _, _, span in interfaces.library.get_spans(self.req, full_trace=True)] | |
| trace = [span for _, _, span in interfaces.library.get_spans(self.req, full_trace=True)] |
tests/test_config_consistency.py
Outdated
|
|
||
| def test_ip_headers_sent_in_one_request(self): | ||
| trace = [span for _, _, span in interfaces.library.get_spans(self.req, full_trace=True)] | ||
| print(trace) |
There was a problem hiding this comment.
Suggested change
| print(trace) | |
| logger.info(spans) |
| weblog_env={ | ||
| "DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING": "false", | ||
| "DD_TRACE_CLIENT_IP_HEADER": "custom-ip-header", | ||
| # disable ASM to test non asm client ip tagging |
There was a problem hiding this comment.
There is a appsec_enabled argument to EndToEndScenario, you can set it to False
mhlidd
requested review from
robertomonteromiguel,
cbeauchesne and
khanayan123
robertomonteromiguel
approved these changes
Dec 13, 2024
cbeauchesne
approved these changes
Dec 13, 2024
simon-id
reviewed
Dec 15, 2024
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Adding tests for tracing configs to make sure implementation across all languages are consistent, based upon the following RFC.
Changes
DD_TRACE_CLIENT_IP_ENABLEDis set tofalseandDD_TRACE_CLIENT_IP_HEADERis specifiedAPMAPI-943
Workflow
codeownersfile quickly.🚀 Once your PR is reviewed, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
[<language>], double-check that only<language>is impacted by the changebuild-XXX-imagelabel is present