Move _dd.apm.enabled tag in tracing Rack middleware

[vpellan]

What does this PR do?
It moves the addition of _dd.apm.enabled tag to spans in tracing's Rack middleware instead of Appsec middleware.

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.

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]

Benchmarks

Benchmark execution time: 2024-11-21 16:25:17

Comparing candidate commit 88b9cd9 in PR branch vpellan/move-dd-apm-enabled-tag with baseline commit 722f487 in branch master.

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

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.78%. Comparing base (722f487) to head (88b9cd9).
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4141      +/-   ##
==========================================
- Coverage   97.79%   97.78%   -0.02%     
==========================================
  Files        1350     1350              
  Lines       81276    81277       +1     
  Branches     4107     4108       +1     
==========================================
- Hits        79481    79473       -8     
- Misses       1795     1804       +9     

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[p-datadog]

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

[TonyCTHsu]

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

[vpellan]

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)

[vpellan]

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 ?

[TonyCTHsu]

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.

1. It is hard to make sense of the convention and logic using Datadog.configuration.tracing.apm.enabled, which APM includes multiple products including profiling.
2. I can see similar approach like rack, that AppSec contains the 3rd party patches that depends on TraceOperation.
3. 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.