Implement Config Consistency #4725

khanayan123 · 2024-09-25T17:23:08Z

What does this PR do?

This pull request updates configuration defaults and behaviors to ensure alignment with standardized definitions across SDKs

changes:

Introduce support for DD_TRACE_<INTEGRATION>_ENABLED, allowing the enablement or disablement of specific integrations based on the provided integration name.
Set the default value for DD_TRACE_RATE_LIMIT to 100.
Add DD_TRACE_ENABLED to the configuration object to enable transmission via telemetry.
Ensure that leading and trailing white spaces are trimmed from items in DD_TRACE_HEADER_TAGS.
Update the behavior of the version tag such that when a span is created with a service name different from the global service name, it will not inherit the global version value.
Update configuration settings listed in the RFC to send telemetry using their respective environment variable names

Motivation

The changes are driven by the need to meet the requirements outlined in the configuration consistency RFC

Additional Notes

This PR was tested against the system tests here

github-actions · 2024-09-25T17:23:52Z

Overall package size

Self size: 7.55 MB
Deduped: 62.29 MB
No deduping: 62.57 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.1.1 | 18.67 MB | 18.68 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.5.0 | 2.51 MB | 2.59 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | lru-cache | 7.14.0 | 74.95 kB | 74.95 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

pr-commenter · 2024-09-26T17:06:57Z

Benchmarks

Benchmark execution time: 2024-10-18 19:50:26

Comparing candidate commit 732d9d1 in PR branch khanayan123/implement-config-consistency with baseline commit 7f93d36 in branch master.

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

codecov · 2024-09-30T03:45:50Z

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.91%. Comparing base (bb0bbcc) to head (040d6bc).
Report is 27 commits behind head on master.

Files with missing lines	Patch %	Lines
packages/dd-trace/src/config.js	66.66%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4725       +/-   ##
===========================================
- Coverage   91.42%   79.91%   -11.51%     
===========================================
  Files         112      303      +191     
  Lines        3475    13096     +9621     
  Branches       33       33               
===========================================
+ Hits         3177    10466     +7289     
- Misses        298     2630     +2332

Flag	Coverage Δ
	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

mtoffl01 · 2024-10-03T13:34:33Z

packages/dd-trace/src/opentracing/tracer.js

+
+    // edge case handling, if a user manually sets a service name different from the tracer's service name
+    // then we need to ensure that the version is set to undefined
+    // in every other scenario version should be set to the tracer's version


I assume by "manually" you mean when creating a custom span and passing a service name != global service name?
Does this apply to integration service names that differ from global service name (since it's not "manual" but does have the "different service name" effect)

@khanayan123 the comment says that we need to do something but why do we need to do it?

Updated the comment, the reason we need to do it is to align with the unified service tagging spec https://docs.datadoghq.com/getting_started/tagging/unified_service_tagging/?tab=kubernetes

DD_VERSION (version) Environment Variable Name: DD_VERSION Behavior: Adds version tag to all spans that have the same service name as the default service name (aka DD_SERVICE). Spans with a different service name do not get this tag.

Configure spans with DD_VERSION to add version to all spans that fall under the service that belongs to the tracer (generally DD_SERVICE). This means that if your service creates spans with the name of an external service, those spans do not receive version as a tag.

Adds version tag to all spans that have the same service name as the default service name (aka DD_SERVICE). Spans with a different service name do not get this tag.

other SDKs such as golang already do this

mtoffl01 · 2024-10-03T13:37:10Z

packages/dd-trace/test/opentracing/tracer.spec.js

@@ -245,6 +245,39 @@ describe('Tracer', () => {
      expect(span.addTags).to.have.been.calledWith(fields.tags)
    })

+    it('If a user sets spans service name differs from the tracers, ensure the spans version is undefined.', () => {


Suggested change

it('If a user sets spans service name differs from the tracers, ensure the spans version is undefined.', () => {

it('If span is granted a service name that differs from the global service name, ensure span's `version` tag is undefined.', () => {

watson

Since this PR changes default values, one could argue it's a breaking change and should be labeled with semver-major. However, to be pragmatic about it, I think it should just be labeled it with semver-minor (currently it's labeled with semver-patch).

packages/dd-trace/src/config.js

tlhunter · 2024-10-11T14:05:01Z

packages/datadog-instrumentations/src/helpers/register.js

+// Check for DD_TRACE_<INTEGRATION>_ENABLED environment variables
+for (const [key, value] of Object.entries(process.env)) {
+  const match = key.match(/^DD_TRACE_(.+)_ENABLED$/)
+  if (match && value.toLowerCase() === 'false') {


In practice customers use both false and 0

tlhunter · 2024-10-11T14:17:03Z

packages/dd-trace/src/config.js

@@ -1144,7 +1147,16 @@ class Config {
    }

    if (typeof value === 'string') {
-      value = value.split(',')
+      // Split by commas and trim each item in header tags mapping
+      if (name === 'headerTags') {


Is there any way to do this in a way that we don't put hyper-specific code, like this header tags check, in a common generic code path?

I think it's fair to remove white spaces around all values in a mapping, going to remove that check for headerTags

tlhunter · 2024-10-11T14:24:43Z

packages/dd-trace/src/opentracing/tracer.js

+
+    // As per unified service tagging spec if a span is created with a service name different from the global
+    // service name it will not inherit the global version value
+    if (options?.tags?.service && options?.tags?.service !== this._service) {


Suggested change

if (options?.tags?.service && options?.tags?.service !== this._service) {

if (options?.tags?.service && options.tags.service !== this._service) {

Since at this point we already know options.tags is an object

tlhunter · 2024-10-11T16:29:16Z

packages/dd-trace/src/config.js

@@ -546,7 +547,7 @@ class Config {
    this._setValue(defaults, 'tracePropagationStyle.otelPropagators', false)
    this._setValue(defaults, 'tracing', true)
    this._setValue(defaults, 'url', undefined)
-    this._setValue(defaults, 'version', pkg.version)


Is this a breaking change? What implications does it come with?

Depends on our definition of breaking change, this shouldn't be a breaking change in the sense that it's part of the UST definition which with this change we would be adhering more closer to, so one could also consider it a patch.

https://docs.datadoghq.com/getting_started/tagging/unified_service_tagging

The implications of this is that unless a 'version' is manually set users will not automatically have version set on their traces, the only caveat being an edge case for dsm to maintain their functionality.

Revert this and I think we can land it

* standardize configurations

khanayan123 added the semver-patch label Oct 1, 2024

khanayan123 changed the title ~~initial commit upstream~~ Implement Config Consistency Oct 2, 2024

khanayan123 marked this pull request as ready for review October 2, 2024 01:49

khanayan123 requested review from a team as code owners October 2, 2024 01:49

khanayan123 requested review from mabdinur, rochdev and bengl October 2, 2024 01:50

mtoffl01 previously approved these changes Oct 3, 2024

View reviewed changes

watson reviewed Oct 7, 2024

View reviewed changes

khanayan123 added semver-minor and removed semver-patch labels Oct 7, 2024

simon-id reviewed Oct 7, 2024

View reviewed changes

packages/dd-trace/src/config.js Show resolved Hide resolved

khanayan123 dismissed mtoffl01’s stale review via 040d6bc October 7, 2024 20:07

khanayan123 requested review from simon-id, watson and tlhunter October 7, 2024 20:11

khanayan123 added 10 commits October 7, 2024 16:12

initial commit upstream

5016df5

update header tags

b2a734f

try

e53f8b3

update

7c5e5e7

fix

d403ccf

try again

139500e

make changes

435d22e

make changes

fbc196d

change 'service' order of precedence

ff91c51

push

78ff456

khanayan123 added 12 commits October 7, 2024 16:12

try

6d45823

add support for DD_TRACE_<INTEGRATION>_ENABLED

ad557a1

try

af340f3

try

cf5b677

make telemetry changes

0907328

fix pr

5c58b7b

get tests passing

ef47b13

push

3cec125

add test

8038021

add test

4afb61a

fix failing tests

37e4cee

update comments

1d50a50

khanayan123 force-pushed the khanayan123/implement-config-consistency branch from 040d6bc to 1d50a50 Compare October 7, 2024 20:14

fix package.json

97af161

tlhunter reviewed Oct 11, 2024

View reviewed changes

khanayan123 added 2 commits October 15, 2024 10:58

address feedback

f0f0b02

update

eae0312

khanayan123 requested a review from tlhunter October 15, 2024 15:03

revert updates to version default

732d9d1

tlhunter approved these changes Oct 21, 2024

View reviewed changes

khanayan123 merged commit 1522a48 into master Oct 22, 2024
198 checks passed

khanayan123 deleted the khanayan123/implement-config-consistency branch October 22, 2024 01:09

rochdev pushed a commit that referenced this pull request Oct 31, 2024

Implement Config Consistency (#4725)

acced78

* standardize configurations

rochdev pushed a commit that referenced this pull request Oct 31, 2024

Implement Config Consistency (#4725)

f11858c

* standardize configurations

rochdev pushed a commit that referenced this pull request Oct 31, 2024

Implement Config Consistency (#4725)

fa10870

* standardize configurations

rochdev pushed a commit that referenced this pull request Nov 6, 2024

Implement Config Consistency (#4725)

c441b02

* standardize configurations

rochdev pushed a commit that referenced this pull request Nov 6, 2024

Implement Config Consistency (#4725)

34d306a

* standardize configurations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Config Consistency #4725

Implement Config Consistency #4725

khanayan123 commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading

pr-commenter bot commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

mtoffl01 Oct 3, 2024

tlhunter Oct 4, 2024

khanayan123 Oct 7, 2024 •

edited

Loading

khanayan123 Oct 7, 2024

mtoffl01 Oct 3, 2024

watson left a comment

tlhunter Oct 11, 2024

tlhunter Oct 11, 2024

khanayan123 Oct 15, 2024

tlhunter Oct 11, 2024

tlhunter Oct 11, 2024

tlhunter Oct 11, 2024

khanayan123 Oct 15, 2024

tlhunter Oct 17, 2024

khanayan123 Oct 18, 2024

	it('If a user sets spans service name differs from the tracers, ensure the spans version is undefined.', () => {
	it('If span is granted a service name that differs from the global service name, ensure span's `version` tag is undefined.', () => {

	if (options?.tags?.service && options?.tags?.service !== this._service) {
	if (options?.tags?.service && options.tags.service !== this._service) {

Implement Config Consistency #4725

Implement Config Consistency #4725

Conversation

khanayan123 commented Sep 25, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

github-actions bot commented Sep 25, 2024 • edited Loading

Overall package size

pr-commenter bot commented Sep 26, 2024 • edited Loading

Benchmarks

codecov bot commented Sep 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanayan123 Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watson left a 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

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

Choose a reason for hiding this comment

khanayan123 commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading

pr-commenter bot commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

khanayan123 Oct 7, 2024 •

edited

Loading