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.

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

Add test to check absence of client computed stats #3812

Merged
merged 5 commits into from
Feb 6, 2025

Conversation

estringana
Copy link
Contributor

@estringana estringana commented Jan 14, 2025

Motivation

The added test is a regression test which caused a incident on PHP. When ASM Standalone was implemented, it was required to send the header Datadog-Client-Computed-Stats as yes when it was a ASM Standalone request. However the way it was implemented in PHP was to add the header Datadog-Client-Computed-Stats always. Then the value of the header was yes when it was a ASM Standalone request and no when not. This was generating the agent to never compute stats as it was interpreting the value no as if it was yes.

Changes

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)?

@estringana estringana force-pushed the estringana/test-asm-standalone-not-enabled branch 8 times, most recently from ebd5622 to ef1e175 Compare January 15, 2025 10:56
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if there is a current test in another scenario that checks this datadog-client-computed-stats header, but IMHO we need one more test case that checks datadog-client-computed-stats=true when standalone billing is disabled and metrics enabled, to ensure the previous is working as before the asm feature was implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I'm following you correctly. How would you make APM to send the header datadog-client-computed-stats=true? In PHP that header was not sent

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's something that only happens in java, but the header is set with empty value, but to keep it simple we can continue with the PR as it is

@estringana estringana force-pushed the estringana/test-asm-standalone-not-enabled branch from 06e9ddb to 39a1b00 Compare January 17, 2025 15:09
@estringana estringana marked this pull request as ready for review January 17, 2025 16:36
@estringana estringana requested review from a team as code owners January 17, 2025 16:36
@estringana estringana force-pushed the estringana/test-asm-standalone-not-enabled branch from 4dacb21 to 9d85bd9 Compare January 22, 2025 17:01
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.

Need to add new scenario in the CI

@estringana estringana force-pushed the estringana/test-asm-standalone-not-enabled branch 2 times, most recently from 3500a27 to e954ad2 Compare January 24, 2025 15:53
@estringana estringana force-pushed the estringana/test-asm-standalone-not-enabled branch from a76ca84 to edb3d81 Compare February 5, 2025 15:32
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.

Failures are not related

@estringana estringana merged commit 5216e27 into main Feb 6, 2025
433 of 436 checks passed
@estringana estringana deleted the estringana/test-asm-standalone-not-enabled branch February 6, 2025 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants