dd-trace-rb icon indicating copy to clipboard operation
dd-trace-rb copied to clipboard

Move `_dd.apm.enabled` tag in tracing root span

Open vpellan opened this issue 1 year ago • 5 comments
trafficstars

What does this PR do?

  • It moves the addition of _dd.apm.enabled tag to spans in tracing root spans.
  • It removes most references to AppSec standalone and replace it by "non_billing" mode (as "APM tracing disabled" can be confusing because this does not completely disables tracing, and we already have a way to completely disable tracing)

It does NOT change the env var DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED to a new one (as the discussion is still ongoing and most likely will stay continue until next quarter)

Motivation: During the review of Standalone SCA system-tests, I found out that enabling 'Standalone Appsec Billing' (DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED=true) and disabling 'Appsec Threats' ( DD_APPSEC_ENABLED=false ) would make the tests fails as _dd.apm.enabled is set in appsec Rack middleware.

This is not correct as this tag is suppose to communicate to the Agent that we should not bill for APM traces, and Standalone Appsec Billing will still limit the number of traces to 1 per minute to keep services alive on the backend side even if Appsec Threats is not enabled, thus the traces must contain that tag. But appsec can have a lot of different service as root span, so we must add it in other integrations too.

Change log entry

None.

How to test the change?

Standalone SCA system-tests (./run.sh SCA_STANDALONE) should XPASS (or pass if force executed)

vpellan avatar Nov 21 '24 15:11 vpellan

Benchmarks

Benchmark execution time: 2024-12-10 15:36:54

Comparing candidate commit eeffb42656b81a2e3f69cb94bbb56884b1f6fc37 in PR branch vpellan/move-dd-apm-enabled-tag with baseline commit c07a8d4cac55ddcea42341787ab5753f910dc162 in branch master.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 30 metrics, 2 unstable metrics.

scenario:profiler - sample timeline=false

  • 🟩 throughput [+0.547op/s; +0.562op/s] or [+9.088%; +9.344%]

pr-commenter[bot] avatar Nov 21 '24 15:11 pr-commenter[bot]

Codecov Report

Attention: Patch coverage is 98.93048% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.78%. Comparing base (c07a8d4) to head (fd7b84c). Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
lib/datadog/tracing/configuration/settings.rb 84.61% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4141      +/-   ##
==========================================
+ Coverage   97.77%   97.78%   +0.01%     
==========================================
  Files        1357     1355       -2     
  Lines       81973    82015      +42     
  Branches     4168     4173       +5     
==========================================
+ Hits        80145    80195      +50     
+ Misses       1828     1820       -8     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Nov 21 '24 16:11 codecov-commenter

As of 27th. of November, the scope of this PR has been changed to moving all "non-billing" mode code to Tracing part. ("Non billing" mode is what was previously referred as "ASM Standalone" mode, that is now referred as "APM Tracing disabled" mode, but since we have already have a different way to completely disable tracing, I refer to it as non-billing mode in this PR to differentiate both, but please note that this is not the official naming for this feature) This is still a WIP (specs not updated) so I changed this PR back to Draft

vpellan avatar Nov 27 '24 18:11 vpellan

Datadog Report

Branch report: vpellan/move-dd-apm-enabled-tag Commit report: eeffb42 Test service: dd-trace-rb

:white_check_mark: 0 Failed, 22057 Passed, 1458 Skipped, 6m 10.58s Total Time :hourglass: 1 Performance Regression

:hourglass: Performance Regressions vs Default Branch (1)

  • loading of products datadog/di loads successfully by itself - rspec 14.75s (+13.48s, +1062%) - Details

:wave: Hey @DataDog/ruby-guild, please fill "Change log entry" section in the pull request description.

If changes need to be present in CHANGELOG.md you can state it this way

**Change log entry**

Yes. A brief summary to be placed into the CHANGELOG.md

(possible answers Yes/Yep/Yeah)

Or you can opt out like that

**Change log entry**

None.

(possible answers No/Nope/None)

Visited at: 2024-12-10 15:07:36 UTC

github-actions[bot] avatar Dec 09 '24 16:12 github-actions[bot]