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

vpellan · 2024-11-21T15:30:19Z

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)

pr-commenter · 2024-11-21T15:56:46Z

Benchmarks

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

Comparing candidate commit eeffb42 in PR branch vpellan/move-dd-apm-enabled-tag with baseline commit c07a8d4 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%]

codecov-commenter · 2024-11-21T16:11:57Z

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 ⚠️

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

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

p-datadog · 2024-11-21T18:33:55Z

lib/datadog/tracing/contrib/rack/middlewares.rb

+            # We add this metric when ASM standalone is enabled to make sure we don't bill APM
+            if Datadog.configuration.appsec.standalone.enabled
+              request_span.set_metric(Tracing::Metadata::Ext::TAG_APM_ENABLED, 0)
+            end


In #3965 @anmarchenko mentioned tracing gaining a dependency on appsec and that pattern appears to continue here. I my opinion it would be better for tracing to provide hooks that appsec will hook into to avoid tracing referencing appsec in code, but I am not developing either part of the library.

I do also find it hard to ascertain whether the code behaves correctly - this PR sets APM enabled to 0 from tracing, but it is not clear from the diff where/under what conditions APM enabled is set to 1 (or if that happens at all).

Can you elaborate the reason about tagging a rack span specifically?

The env var name DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED is temporary, and what Standalone Appsec actually does is disabling tracing (or more precisely rate-limiting it at 1 trace per minute), and when 'Appsec' is activated along 'Standalone Appsec' (as Standalone Appsec does not in fact activate Appsec by itself), it force-keep traces that contains Appsec events.

There is a RFC currently being reviewed to find the final name for this env var, which will most likely be DD_TRACE_APM_ENABLED. When it will be approved, I will also change the configuration from Datadog.configuration.appsec.standalone.enabled to something like Datadog.configuration.tracing.apm.enabled, which will make more sense and show that there is no dependency from appsec in tracing (but the other way will be true, which already is anyway)

Can you elaborate the reason about tagging a rack span specifically?

For now, the only product other than tracing that uses traces is Appsec, and in Appsec we only instrument rack-based apps (Rails, Sinatra, Rack itself...)
But maybe we could tag somewhere else, like in http frameworks, now that we have moved this part in tracing. What do you think of adding that tag on the HTTP level ? Or should we stick to Rack, and maybe also add other frameworks like sidekiq ?

I understand this is experimental feature but similar concern with shotgun surgery and product coupling has been raised in the previous pull request code review, that signals the severity of design issue.

It is hard to make sense of the convention and logic using Datadog.configuration.tracing.apm.enabled, which APM includes multiple products including profiling.

I can see similar approach like rack, that AppSec contains the 3rd party patches that depends on TraceOperation.

Or maybe other kinds of dependency injection mechanism

Overall, the requirements (some kinds of hook systems, perhaps could be encompassed by @lloeki 's Instrumentation API ?) are not clear and I am afraid that if we are indifferent now could cast a long shadow in the future.

does it make sense to find the root span here and not the rack span?

Yes, this is changed in e0c503a

Note that "non-billing" naming is temporary.

y9v · 2024-11-27T08:21:43Z

.github/workflows/system-tests.yml

@@ -11,7 +11,7 @@ on:
 env:
  REGISTRY: ghcr.io
  REPO: ghcr.io/datadog/dd-trace-rb
-  SYSTEM_TESTS_REF: main # This must always be set to `main` on dd-trace-rb's master branch
+  SYSTEM_TESTS_REF: igor/standalone-sca # This must always be set to `main` on dd-trace-rb's master branch


reminder: please change this branch to master before merging

y9v · 2024-11-27T08:23:48Z

lib/datadog/tracing/contrib/rack/middlewares.rb

+            # We add this metric when ASM standalone is enabled to make sure we don't bill APM
+            if Datadog.configuration.appsec.standalone.enabled
+              request_span.set_metric(Tracing::Metadata::Ext::TAG_APM_ENABLED, 0)
+            end


does it make sense to find the root span here and not the rack span?

vpellan · 2024-11-27T18:28:22Z

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

datadog-datadog-prod-us1 · 2024-12-02T14:15:21Z

Datadog Report

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

✅ 0 Failed, 22057 Passed, 1458 Skipped, 6m 10.58s Total Time
⌛ 1 Performance Regression

⌛ Performance Regressions vs Default Branch (1)

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

…ec middleware

…LONE_ENABLED

… non-billing mode

…ive trace

…lity

github-actions · 2024-12-09T16:59:29Z

👋 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}

…ect? spec

p-datadog · 2024-12-11T20:36:37Z

lib/datadog/tracing/trace_operation.rb

+      # unless specific distributed tags are present. These tags can come from upstream
+      # services, or from the service itself.
+      # For now, only '_dd.p.appsec=1' is supported.
+      def non_billing_reject?


This method name contains two negatives. I would prefer something like should_send? for example but before renaming let's figure out the overall name for the setting.

Strech · 2024-12-12T08:52:33Z

lib/datadog/tracing/configuration/ext.rb

@@ -12,6 +12,7 @@ module Ext
        ENV_OTEL_TRACES_EXPORTER = 'OTEL_TRACES_EXPORTER'
        ENV_HEADER_TAGS = 'DD_TRACE_HEADER_TAGS'
        ENV_TRACE_ID_128_BIT_GENERATION_ENABLED = 'DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED'
+        ENV_APM_ENABLED = 'DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED' # Temporary until RFC decides final name


Could we highlight that "temporary" nature in the name? It's a big ask to name it ENV_APM_ENABLED, but actually check for standalone feature

P.S let's do it only if it makes sense (merged before RFC is in final state)

vpellan requested review from a team as code owners November 21, 2024 15:30

github-actions bot added integrations Involves tracing integrations appsec Application Security monitoring product tracing labels Nov 21, 2024

vpellan requested a review from a team as a code owner November 21, 2024 15:47

p-datadog approved these changes Nov 21, 2024

View reviewed changes

y9v reviewed Nov 27, 2024

View reviewed changes

vpellan marked this pull request as draft November 27, 2024 17:46

github-actions bot added the core Involves Datadog core libraries label Nov 27, 2024

vpellan changed the title ~~Move _dd.apm.enabled tag in tracing Rack middleware~~ Move _dd.apm.enabled tag in tracing root span Nov 28, 2024

vpellan force-pushed the vpellan/move-dd-apm-enabled-tag branch from 7ee5c9f to e6639a4 Compare December 2, 2024 14:02

vpellan force-pushed the vpellan/move-dd-apm-enabled-tag branch 2 times, most recently from 2061cce to e8acbf8 Compare December 2, 2024 14:34

vpellan added 11 commits December 9, 2024 11:26

Move _dd.apm.enabled tag in Tracing Rack Middleware instead of AppS…

51585fc

…ec middleware

Add SCA_STANDALONE scenario and force-execute SCA_STANDALONE tests

7e1d748

Changed c.appsec.standalone.enabled to c.tracing.apm.enabled

6433166

Reverse config while we are still using DD_EXPERIMENTAL_APPSEC_STANDA…

dd79a68

…LONE_ENABLED

Use TraceOperation method in should_skip_distributed_tracing

85e116f

Add _dd.apm.enabled: 0 to root spans instead of rack spans when using…

cc4be80

… non-billing mode

Renamed appsec_standalone_reject? to non_billing_reject?

cc72ea5

Lint

c001f58

Fix should_skip_distributed_tracing with non-billing mode when no act…

8e7f016

…ive trace

Un-factorized shoukd_skip_distributed_tracing for better code readabi…

e25c644

…lity

fix should_skip_distributed_tracing? methods to use trace

bf8190b

vpellan added 7 commits December 9, 2024 11:26

Fix "should_skip_distributed_tracing?" in contrib

f702fe9

non-billing mode spec

fc68c33

switch tests values for apm enabled

3c39598

Fix error on should_skip_distributed_tracing? on ethon instrumentation

c7efe5c

Fix gRPC non-billing mode test

3d54594

Fix gRPC specs

7e8cf81

Remove older should_skip_distributed_tracing?

3142efb

vpellan force-pushed the vpellan/move-dd-apm-enabled-tag branch from c28f69f to 3142efb Compare December 9, 2024 13:00

vpellan added 3 commits December 10, 2024 13:48

Replace tracing_apm_enabled by non_billing_enabled in non_billing_rej…

22a9b40

…ect? spec

Replaced c.tracing.apm.enabled to c.tracing.non_billing.enabled

fd7b84c

Lint

eeffb42

vpellan force-pushed the vpellan/move-dd-apm-enabled-tag branch from 84c2916 to eeffb42 Compare December 10, 2024 15:13

vpellan marked this pull request as ready for review December 10, 2024 15:19

vpellan requested a review from p-datadog December 11, 2024 12:20

p-datadog reviewed Dec 11, 2024

View reviewed changes

Strech reviewed Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vpellan commented Nov 21, 2024 •

edited

Loading

pr-commenter bot commented Nov 21, 2024 •

edited

Loading

codecov-commenter commented Nov 21, 2024 •

edited

Loading

p-datadog Nov 21, 2024

TonyCTHsu Nov 22, 2024

vpellan Nov 22, 2024

vpellan Nov 22, 2024 •

edited

Loading

TonyCTHsu Nov 22, 2024 •

edited

Loading

y9v Nov 27, 2024

vpellan Nov 27, 2024

y9v Nov 27, 2024

y9v Nov 27, 2024

vpellan commented Nov 27, 2024 •

edited

Loading

datadog-datadog-prod-us1 bot commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 9, 2024 •

edited

Loading

p-datadog Dec 11, 2024

Strech Dec 12, 2024

Move _dd.apm.enabled tag in tracing root span #4141

Are you sure you want to change the base?

Move _dd.apm.enabled tag in tracing root span #4141

Conversation

vpellan commented Nov 21, 2024 • edited Loading

pr-commenter bot commented Nov 21, 2024 • edited Loading

Benchmarks

scenario:profiler - sample timeline=false

codecov-commenter commented Nov 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpellan Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

TonyCTHsu Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpellan commented Nov 27, 2024 • edited Loading

datadog-datadog-prod-us1 bot commented Dec 2, 2024 • edited Loading

Datadog Report

⌛ Performance Regressions vs Default Branch (1)

github-actions bot commented Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

vpellan commented Nov 21, 2024 •

edited

Loading

pr-commenter bot commented Nov 21, 2024 •

edited

Loading

codecov-commenter commented Nov 21, 2024 •

edited

Loading

vpellan Nov 22, 2024 •

edited

Loading

TonyCTHsu Nov 22, 2024 •

edited

Loading

vpellan commented Nov 27, 2024 •

edited

Loading

datadog-datadog-prod-us1 bot commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 9, 2024 •

edited

Loading