[Debugger Default-On] DEBUG-4406 Support multi-config merging with priorities by dudikeleti · Pull Request #7536 · DataDog/dd-trace-dotnet

dudikeleti · 2025-09-18T18:56:41Z

Summary of changes

Implements priority-based merging for APM_TRACING config with determenistic order:
Priority Order: Service+Env (4) > Service (3) > Env (2) > Wildcard (1) > Org (0)

Reason for change

When multiple configs are sent, the "last one wins." This causes non-deterministic behavior across tracers and inconsistent feature enablement. internal RFC

Test coverage

ApmTracingConfigMergerTests.cs
DataDog/system-tests#5338

dd-trace-dotnet-ci-bot · 2025-09-18T19:49:40Z

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

Welch test with statistical test for significance of 5%
Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7536) - mean (72ms)  : 71, 73
     .   : milestone, 72,
    master - mean (72ms)  : 71, 73
     .   : milestone, 72,

    section Baseline
    This PR (7536) - mean (68ms)  : 66, 71
     .   : milestone, 68,
    master - mean (68ms)  : 65, 71
     .   : milestone, 68,

    section CallTarget+Inlining+NGEN
    This PR (7536) - mean (1,053ms)  : 989, 1116
     .   : milestone, 1053,
    master - mean (1,044ms)  : 1011, 1076
     .   : milestone, 1044,

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7536) - mean (107ms)  : 105, 108
     .   : milestone, 107,
    master - mean (106ms)  : 105, 108
     .   : milestone, 106,

    section Baseline
    This PR (7536) - mean (106ms)  : 104, 109
     .   : milestone, 106,
    master - mean (106ms)  : 104, 108
     .   : milestone, 106,

    section CallTarget+Inlining+NGEN
    This PR (7536) - mean (743ms)  : 723, 764
     .   : milestone, 743,
    master - mean (746ms)  : 727, 764
     .   : milestone, 746,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7536) - mean (101ms)  : 100, 102
     .   : milestone, 101,
    master - mean (100ms)  : 100, 101
     .   : milestone, 100,

    section Baseline
    This PR (7536) - mean (100ms)  : 98, 102
     .   : milestone, 100,
    master - mean (100ms)  : 98, 103
     .   : milestone, 100,

    section CallTarget+Inlining+NGEN
    This PR (7536) - mean (768ms)  : 711, 825
     .   : milestone, 768,
    master - mean (775ms)  : 723, 827
     .   : milestone, 775,

gantt
    title Execution time (ms) FakeDbCommand (.NET 8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7536) - mean (93ms)  : 92, 94
     .   : milestone, 93,
    master - mean (93ms)  : 92, 94
     .   : milestone, 93,

    section Baseline
    This PR (7536) - mean (92ms)  : 90, 94
     .   : milestone, 92,
    master - mean (92ms)  : 89, 95
     .   : milestone, 92,

    section CallTarget+Inlining+NGEN
    This PR (7536) - mean (661ms)  : 643, 680
     .   : milestone, 661,
    master - mean (664ms)  : 649, 678
     .   : milestone, 664,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7536) - mean (203ms)  : 197, 209
     .   : milestone, 203,
    master - mean (200ms)  : 197, 204
     .   : milestone, 200,

    section Baseline
    This PR (7536) - mean (200ms)  : 193, 206
     .   : milestone, 200,
    master - mean (200ms)  : 189, 211
     .   : milestone, 200,

    section CallTarget+Inlining+NGEN
    This PR (7536) - mean (1,189ms)  : 1114, 1263
     .   : milestone, 1189,
    master - mean (1,199ms)  : 1130, 1268
     .   : milestone, 1199,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7536) - mean (287ms)  : 280, 294
     .   : milestone, 287,
    master - mean (286ms)  : 277, 296
     .   : milestone, 286,

    section Baseline
    This PR (7536) - mean (287ms)  : 279, 295
     .   : milestone, 287,
    master - mean (283ms)  : 275, 291
     .   : milestone, 283,

    section CallTarget+Inlining+NGEN
    This PR (7536) - mean (954ms)  : 909, 998
     .   : milestone, 954,
    master - mean (950ms)  : 902, 998
     .   : milestone, 950,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7536) - mean (290ms)  : 282, 298
     .   : milestone, 290,
    master - mean (289ms)  : 277, 301
     .   : milestone, 289,

    section Baseline
    This PR (7536) - mean (290ms)  : 281, 298
     .   : milestone, 290,
    master - mean (288ms)  : 280, 296
     .   : milestone, 288,

    section CallTarget+Inlining+NGEN
    This PR (7536) - mean (1,022ms)  : 973, 1070
     .   : milestone, 1022,
    master - mean (1,011ms)  : 953, 1069
     .   : milestone, 1011,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7536) - mean (282ms)  : 272, 293
     .   : milestone, 282,
    master - mean (276ms)  : 267, 285
     .   : milestone, 276,

    section Baseline
    This PR (7536) - mean (281ms)  : 269, 292
     .   : milestone, 281,
    master - mean (278ms)  : 269, 287
     .   : milestone, 278,

    section CallTarget+Inlining+NGEN
    This PR (7536) - mean (882ms)  : 847, 917
     .   : milestone, 882,
    master - mean (875ms)  : 847, 902
     .   : milestone, 875,

tracer/src/Datadog.Trace/RemoteConfigurationManagement/RcmCapabilitiesIndices.cs

tracer/src/Datadog.Trace/Configuration/DynamicConfigurationManager.cs

anna-git · 2025-09-24T10:59:22Z

tracer/src/Datadog.Trace/Configuration/ApmTracingConfig.cs

+            var higherPriority = this.Priority >= other.Priority ? this : other;
+            var lowerPriority = this.Priority >= other.Priority ? other : this;


nit

Suggested change

var higherPriority = this.Priority >= other.Priority ? this : other;

var lowerPriority = this.Priority >= other.Priority ? other : this;

var (higherPriority, lowerPriority) = this.Priority >= other.Priority ? (this, other) : (other, this);

I like it too but does not work in .net461 (am I missing something?)

I think this syntax uses ValueTuple which was added in net47.

ah yes true 😓 , I meant also not testing twice this.Priority >= other.Priority but it's very nit

anna-git · 2025-09-24T11:10:00Z

tracer/src/Datadog.Trace/Configuration/DynamicConfigurationManager.cs

+        {
+            // Get current service/environment for filtering
+            var currentSettings = Tracer.Instance.Settings;
+            var serviceName = currentSettings.ServiceName ?? "unknown";


do we want the DefaultServiceName here or the provided service name through DD_SERVICE? cc @andrewlock

Fixed in d29f916

anna-git · 2025-09-24T11:45:01Z

tracer/src/Datadog.Trace/Configuration/DynamicConfigurationManager.cs


-                return result;
-            }
+            var mergedConfigJson = ApmTracingConfigMerger.MergeConfigurations(


in MergeConfigurations we deserialize and reserialize something that's gonna be deserialized again within DynamicConfigConfigurationSource. Could we not integrate the merge logic inside DynamicConfigConfiguratioSource directly? like around here

dd-trace-dotnet/tracer/src/Datadog.Trace/Configuration/ConfigurationSources/DynamicConfigConfigurationSource.cs

Line 43 in 7e24fb2

if (jobject != null)

this way we could deserialize once and treat the configurations directly

Please see updated solution 66af9cf
I Can integrate it into DynamicConfigConfigurationSource but I'm afraid that would be too much for configuratioin sousrce. Anyway let me know WDYT

bouwkast

There are discrepancies between the implementations

Can we get those looked at (or maybe I'm missing something here as I'm not the best at going through other code)?

tracer/src/Datadog.Trace/Configuration/ApmTracingConfig.cs

tracer/src/Datadog.Trace/Configuration/ApmTracingConfigMerger.cs

tracer/src/Datadog.Trace/Configuration/ApmTracingConfig.cs

bouwkast

LGTM code wise but I'm unsure exactly why the benchmarks went up so much they are usually quite variable and / or consistent so them all increasing seems a bit worrying

Maybe associated with the job failures also in GitLab here?

…dd more tests

…igurationSource

bouwkast

👍

## Summary of changes This PR finalizes the work introduced in [Debugger In-Product Enablement](#7366) and [RC Multi-Config Support](#7536) by integrating the new debugger dynamic configuration keys into both the dynamic configuration source and the APM tracing merger. ## Test coverage DebuggerManagerDynamicTests

## Summary of changes - Adds some unit tests for some of the `APM_TRACING` logic in `DynamicConfigurationManager` - ~Reduce duration of `lock`~ Remove `lock` - Minor reduction in allocations ## Reason for change Rebasing #7525 after #7536 flagged that there were some conflicts. In working out how to resolve them, noted some possibilities for minor improvements ## Implementation details - Extracted existing functionality into a testable method - Added unit tests for the existing behaviour - Tweaks Technically I believe this is _always_ single-threaded, so we _should_ be able to remove the `lock` and also reduce some `ToList()` etc, but we don't _explicitly_ enforce that, so I'm not sure whether to do it or not tbh 🤔 Thoughts? Given [this comment](https://github.com/apps/chatgpt-codex-connector) I'm inclined to remove it entirely. EDIT: we removed the lock entirely in the end for simplicity ## Test coverage More now ## Other details Reviewing commit-by-commit shows the changes more clearly 🙂

dudikeleti force-pushed the dudik/multi-config branch from 690a914 to 416e8ac Compare September 20, 2025 00:43

This comment has been minimized.

Sign in to view

dudikeleti marked this pull request as ready for review September 22, 2025 12:34

dudikeleti requested a review from a team as a code owner September 22, 2025 12:34

dudikeleti requested review from anna-git and removed request for anna-git September 23, 2025 14:06

dudikeleti force-pushed the dudik/multi-config branch from 19c9898 to 10a9ba8 Compare September 23, 2025 18:09

dudikeleti changed the title ~~[ER Default-On] DEBUG-4406 Support multi config mering with priorities~~ [Debugger Default-On] DEBUG-4406 Support multi config mering with priorities Sep 24, 2025

anna-git reviewed Sep 24, 2025

View reviewed changes

tracer/src/Datadog.Trace/RemoteConfigurationManagement/RcmCapabilitiesIndices.cs Show resolved Hide resolved

anna-git reviewed Sep 24, 2025

View reviewed changes

tracer/src/Datadog.Trace/Configuration/DynamicConfigurationManager.cs Outdated Show resolved Hide resolved

anna-git reviewed Sep 24, 2025

View reviewed changes

bouwkast requested changes Sep 24, 2025

View reviewed changes

lucaspimentel changed the title ~~[Debugger Default-On] DEBUG-4406 Support multi config mering with priorities~~ [Debugger Default-On] DEBUG-4406 Support multi config merging with priorities Sep 24, 2025

lucaspimentel changed the title ~~[Debugger Default-On] DEBUG-4406 Support multi config merging with priorities~~ [Debugger Default-On] DEBUG-4406 Support multi-config merging with priorities Sep 24, 2025

github-actions bot added the area:debugger label Sep 24, 2025

dudikeleti force-pushed the dudik/multi-config branch 2 times, most recently from aaca066 to 308e398 Compare September 29, 2025 13:11

bouwkast self-requested a review September 29, 2025 19:51

bouwkast reviewed Sep 29, 2025

View reviewed changes

dudikeleti added 8 commits September 30, 2025 12:31

support multi config with priorities

de7e5c3

update capabilities indices

79fab55

handle configuration removal, add cluster, fix multi config history a…

671edbe

…dd more tests

add missing capability

c4f2d7b

Fix DynamicConfigurationTests

bf1f241

Fix UpdateConfiguration test

5a64ae4

Add the rest of LibConfig properties to the merge logic

b4d37f0

Handle more priorities combinations

473994c

dudikeleti added 7 commits September 30, 2025 12:31

Do not filter un-matched service name or environment name

2c0a82e

make the sorting more understandable and add early exit

d8813fe

Fix comment

8b37c4a

Add a way for providing jtoken instead of string to DynamicConfigConf…

c8d3092

…igurationSource

Update tests

6ece5af

Set service name to DefaultServieNamein case DD_SERVICE omitted

60631ac

indentation

c7afdd4

dudikeleti force-pushed the dudik/multi-config branch from 91cf706 to c7afdd4 Compare September 30, 2025 10:32

bouwkast self-requested a review September 30, 2025 14:50

bouwkast approved these changes Sep 30, 2025

View reviewed changes

dudikeleti merged commit a220a59 into master Sep 30, 2025
135 of 150 checks passed

dudikeleti deleted the dudik/multi-config branch September 30, 2025 15:57

github-actions bot added this to the vNext-v3 milestone Sep 30, 2025

dudikeleti mentioned this pull request Oct 1, 2025

[Debugger Default-On] Add debuger dynamic configurations keys #7595

Merged

andrewlock mentioned this pull request Oct 14, 2025

Minor refactor and added tests for DynamicConfigurationManager #7652

Merged

		var higherPriority = this.Priority >= other.Priority ? this : other;
		var lowerPriority = this.Priority >= other.Priority ? other : this;

	var higherPriority = this.Priority >= other.Priority ? this : other;
	var lowerPriority = this.Priority >= other.Priority ? other : this;
	var (higherPriority, lowerPriority) = this.Priority >= other.Priority ? (this, other) : (other, this);

Conversation

dudikeleti commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Test coverage

Uh oh!

dd-trace-dotnet-ci-bot bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Execution-Time Benchmarks Report ⏱️

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

anna-git Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dudikeleti Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

lucaspimentel Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

anna-git Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

anna-git Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

dudikeleti Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

anna-git Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

dudikeleti Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

bouwkast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bouwkast left a comment

Choose a reason for hiding this comment

Uh oh!

bouwkast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dudikeleti commented Sep 18, 2025 •

edited

Loading

dd-trace-dotnet-ci-bot bot commented Sep 18, 2025 •

edited

Loading

anna-git Sep 24, 2025 •

edited

Loading