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

config_consistency: test ClientIPHeader #3045

Merged
merged 10 commits into from
Sep 19, 2024
Merged

config_consistency: test ClientIPHeader #3045

merged 10 commits into from
Sep 19, 2024

Conversation

mabdinur
Copy link
Contributor

@mabdinur mabdinur commented Sep 16, 2024 •
edited
Loading

Description

  • Introduces a test that validates that the DD_TRACE_CLIENT_IP_HEADER configuration is supported across client libraries
  • Enables this test for the following libraries: python, ...

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • If PR title starts with [<language>], double-check that only <language> is impacted by the change
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

@mabdinur mabdinur force-pushed the munir/APMAPI-289-DD_TRACE_OBFUSCATION_QUERY_STRING_REGEXP branch from 6f59042 to 29901e2 Compare September 16, 2024 16:31
Base automatically changed from munir/APMAPI-289-DD_TRACE_OBFUSCATION_QUERY_STRING_REGEXP to main September 17, 2024 16:06
@mabdinur mabdinur force-pushed the APMAPI-351/munir-test-ip-header-consistency branch 7 times, most recently from a1ceb0c to 05ab289 Compare September 17, 2024 20:51
@mabdinur mabdinur force-pushed the APMAPI-351/munir-test-ip-header-consistency branch from 05ab289 to cf028ed Compare September 17, 2024 21:05
@mabdinur mabdinur force-pushed the APMAPI-351/munir-test-ip-header-consistency branch from 81a740d to f3abf75 Compare September 18, 2024 02:29
@mabdinur mabdinur force-pushed the APMAPI-351/munir-test-ip-header-consistency branch from f3abf75 to bba8ac0 Compare September 18, 2024 02:31
mabdinur
mabdinur commented Sep 18, 2024
View reviewed changes
tests/test_config_consistency.py
@@ -178,33 +178,46 @@ def test_query_string_redaction(self):
assert _get_span_by_tags(trace, expected_tags), f"Span with tags {expected_tags} not found in {trace}"


def _get_span(spans, resource_name, tags):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zacharycmontoya I replaced _get_span with _get_span_by_tags. I found that this approach works best with most libraries. Resource names are not set in a consistent way across client libraries. This is something we can work with IDM to address.

@mabdinur mabdinur marked this pull request as ready for review September 18, 2024 03:18
@mabdinur mabdinur requested review from a team as code owners September 18, 2024 03:18
@mabdinur mabdinur requested review from a team as code owners September 18, 2024 03:18
@mabdinur mabdinur requested review from tabgok, P403n1x87, smola, ValentinZakharov and dmehala and removed request for a team September 18, 2024 03:18
cbeauchesne
cbeauchesne requested changes Sep 18, 2024
View reviewed changes
Copy link
Collaborator

@cbeauchesne cbeauchesne left a comment

Choose a reason for hiding this comment

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

Just a small typo.

manifests/php.yml Outdated Show resolved Hide resolved
@mabdinur mabdinur force-pushed the APMAPI-351/munir-test-ip-header-consistency branch from 5561255 to 92331ea Compare September 18, 2024 15:49
simon-id
simon-id reviewed Sep 19, 2024
View reviewed changes
manifests/nodejs.yml Outdated Show resolved Hide resolved
mabdinur
mabdinur commented Sep 19, 2024
View reviewed changes
manifests/nodejs.yml Outdated Show resolved Hide resolved
@mabdinur
Copy link
Contributor Author

CI failures are unreleated (ex: due to mysql issues)

@mabdinur mabdinur merged commit 5ca3d3c into main Sep 19, 2024
372 of 386 checks passed
@mabdinur mabdinur deleted the APMAPI-351/munir-test-ip-header-consistency branch September 19, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simon-id simon-id simon-id left review comments

@cbeauchesne cbeauchesne cbeauchesne approved these changes

@tabgok tabgok Awaiting requested review from tabgok tabgok is a code owner automatically assigned from DataDog/apm-python

@P403n1x87 P403n1x87 Awaiting requested review from P403n1x87 P403n1x87 is a code owner automatically assigned from DataDog/apm-python

@smola smola Awaiting requested review from smola smola is a code owner automatically assigned from DataDog/asm-java

@ValentinZakharov ValentinZakharov Awaiting requested review from ValentinZakharov ValentinZakharov is a code owner automatically assigned from DataDog/asm-java

@dmehala dmehala Awaiting requested review from dmehala dmehala is a code owner automatically assigned from DataDog/dd-trace-cpp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mabdinur @cbeauchesne @simon-id