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

chore(asm): standalone asm propagation #9482

Merged
merged 31 commits into from
Jun 10, 2024

Conversation

gnufede
Copy link
Member

@gnufede gnufede commented Jun 5, 2024

Description:

ASM: adds Standalone ASM distributed propagation changes as described in "RFC: Standalone ASM billing V2".

For the full picture of this feature, see: #9211 , #9444 and #9445

See also System Tests related changes: DataDog/system-tests#2522

Details:

The main change is that if ASM Standalone is enabled, propagation of distributed spans would reset (from upstream) unless they are part of a distributed span where there are AppSec events (signaled through the propagation tag _dd.p.appsec: 1).

It will also cut propagation downstream if there are no appsec events in the current or upstream spans.

Notice that AppSec events trigger a force keep, and that takes precedence over the received propagation in this PR.

Also notice that most tests start by creating a first span without an appsec event. This is due to the fact that ASM Standalone needs to maintain a minimum rate of 1 trace per minute regardless of upstream propagation or appsec events present, so that way we are not affected by that rate in the test.

Checklist

  • Change(s) are motivated and described in the PR description
  • Testing strategy is described if automated tests are not included in the PR
  • Risks are described (performance impact, potential for breakage, maintainability)
  • Change is maintainable (easy to change, telemetry, documentation)
  • Library release note guidelines are followed or label changelog/no-changelog is set
  • Documentation is included (in-code, generated user docs, public corp docs)
  • Backport labels are set (if applicable)
  • If this PR changes the public interface, I've notified @DataDog/apm-tees.

Reviewer Checklist

  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Description motivates each change
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Change is maintainable (easy to change, telemetry, documentation)
  • Release note makes sense to a user of the library
  • 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

@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Jun 5, 2024

Datadog Report

Branch report: gnufede/APPSEC-53002-standalone-asm-propagation-2
Commit report: ce88f01
Test service: dd-trace-py

✅ 0 Failed, 118967 Passed, 55776 Skipped, 3h 33m 53.16s Total duration (5h 11m 8.11s time saved)

@gnufede gnufede added changelog/no-changelog A changelog entry is not required for this PR. ASM Application Security Monitoring labels Jun 6, 2024
@gnufede gnufede changed the title chore(asm): standalone asm propagation - second variant chore(asm): standalone asm propagation - second approach Jun 6, 2024
@pr-commenter
Copy link

pr-commenter bot commented Jun 6, 2024

Benchmarks

Benchmark execution time: 2024-06-10 14:23:24

Comparing candidate commit ce88f01 in PR branch gnufede/APPSEC-53002-standalone-asm-propagation-2 with baseline commit b0fdb1e in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 221 metrics, 9 unstable metrics.

@gnufede gnufede requested a review from Julio-Guerra June 6, 2024 09:12
@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 2.12766% with 184 lines in your changes missing coverage. Please review.

Project coverage is 10.27%. Comparing base (844117e) to head (ce88f01).
Report is 3 commits behind head on main.

Files Patch % Lines
tests/tracer/test_propagation.py 0.00% 176 Missing ⚠️
ddtrace/propagation/http.py 22.22% 7 Missing ⚠️
ddtrace/_trace/tracer.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9482       +/-   ##
===========================================
- Coverage   75.88%   10.27%   -65.61%     
===========================================
  Files        1315     1285       -30     
  Lines      124706   123093     -1613     
===========================================
- Hits        94633    12650    -81983     
- Misses      30073   110443    +80370     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gnufede gnufede marked this pull request as ready for review June 6, 2024 14:31
@gnufede gnufede requested review from a team as code owners June 6, 2024 14:31
@gnufede gnufede requested a review from brettlangdon June 6, 2024 14:31
@gnufede gnufede marked this pull request as draft June 7, 2024 08:21
@gnufede gnufede marked this pull request as ready for review June 7, 2024 14:20
@gnufede gnufede changed the title chore(asm): standalone asm propagation - second approach chore(asm): standalone asm propagation Jun 10, 2024
@gnufede gnufede enabled auto-merge (squash) June 10, 2024 13:25
@gnufede gnufede merged commit 20e87ca into main Jun 10, 2024
224 of 230 checks passed
@gnufede gnufede deleted the gnufede/APPSEC-53002-standalone-asm-propagation-2 branch June 10, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASM Application Security Monitoring changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants