[Config Registry] 7/8 Integration names to keys #7937
Conversation
dad2367 to
f18f8e4
Compare
e1c988c to
ccacb2d
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7937) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-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 highlighted in **red**. The following thresholds were used for comparing the execution times:
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). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (69ms) : 67, 70
master - mean (68ms) : 67, 70
section Bailout
This PR (7937) - mean (73ms) : 71, 74
master - mean (72ms) : 72, 73
section CallTarget+Inlining+NGEN
This PR (7937) - mean (1,026ms) : 960, 1091
master - mean (1,020ms) : 931, 1109
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (106ms) : 103, 109
master - mean (106ms) : 103, 108
section Bailout
This PR (7937) - mean (107ms) : 106, 108
master - mean (107ms) : 106, 108
section CallTarget+Inlining+NGEN
This PR (7937) - mean (747ms) : 697, 798
master - mean (734ms) : 673, 795
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (94ms) : 92, 96
master - mean (94ms) : 92, 96
section Bailout
This PR (7937) - mean (94ms) : 93, 95
master - mean (94ms) : 93, 95
section CallTarget+Inlining+NGEN
This PR (7937) - mean (721ms) : 670, 772
master - mean (719ms) : 686, 753
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (92ms) : 90, 95
master - mean (92ms) : 90, 94
section Bailout
This PR (7937) - mean (93ms) : 92, 94
master - mean (93ms) : 92, 94
section CallTarget+Inlining+NGEN
This PR (7937) - mean (639ms) : 627, 652
master - mean (637ms) : 618, 655
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (194ms) : 191, 197
master - mean (195ms) : 190, 199
section Bailout
This PR (7937) - mean (198ms) : 194, 202
master - mean (197ms) : 195, 200
section CallTarget+Inlining+NGEN
This PR (7937) - mean (1,153ms) : 1081, 1225
master - mean (1,130ms) : 1063, 1197
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (278ms) : 271, 285
master - mean (280ms) : 272, 288
section Bailout
This PR (7937) - mean (279ms) : 275, 282
master - mean (278ms) : 275, 281
section CallTarget+Inlining+NGEN
This PR (7937) - mean (942ms) : 898, 986
master - mean (940ms) : 905, 976
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (272ms) : 267, 277
master - mean (271ms) : 266, 277
section Bailout
This PR (7937) - mean (271ms) : 267, 275
master - mean (271ms) : 267, 274
section CallTarget+Inlining+NGEN
This PR (7937) - mean (932ms) : 882, 983
master - mean (919ms) : 868, 969
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (270ms) : 265, 276
master - mean (270ms) : 264, 276
section Bailout
This PR (7937) - mean (270ms) : 266, 274
master - mean (270ms) : 266, 275
section CallTarget+Inlining+NGEN
This PR (7937) - mean (841ms) : 822, 860
master - mean (834ms) : 812, 856
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
f18f8e4 to
303b602
Compare
ccacb2d to
cf34d67
Compare
303b602 to
70df7c9
Compare
cf34d67 to
c8caa83
Compare
This comment has been minimized.
This comment has been minimized.
…nes in the whole solution (#7931) (This PR is actually #7697 that accidentally got closed) ## Context Part of **Configuration Inversion (Step 5)** - Stack progress: 1. [#7548](#7548) - Add GitLab step and JSON configuration file 2. [#7688](#7688) - Cleanup configuration / platform keys + source generator 3. [#7698](#7698) - Aliases handling via source generator 4. [#7689](#7689) - Analyzers for platform and ConfigurationBuilder 5. **→ [#7931](#7697) - Replace manual ConfigurationKeys by generated ones in the whole solution (this PR)** 6. [#7932](#7932) - Forbid use of System.Environment methods and adapt everywhere 7. [#7937](#7937) - Integration names to generated keys ## Summary of changes Fixed the [ConfigurationKeysGenerator](cci:2://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace.SourceGenerators/Configuration/ConfigurationKeysGenerator.cs:21:0-958:1) to properly read and apply the [configuration_keys_mapping.json](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/configuration_keys_mapping.json:0:0-0:0) file, and extracted common file header comments to a reusable constant. **Key changes:** - Fixed JSON array extraction in [ParseMappingFile](cci:1://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace.SourceGenerators/Configuration/ConfigurationKeysGenerator.cs:350:4-524:5) method - the generator was incorrectly trying to extract the `"mappings"` field as an object instead of an array - Extracted configuration generator comments to `Constants.ConfigurationGeneratorComment` for reuse across multiple generators - Updated both [ConfigurationKeysGenerator](cci:2://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace.SourceGenerators/Configuration/ConfigurationKeysGenerator.cs:21:0-958:1) and [ConfigKeyAliasesSwitcherGenerator](cci:2://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace.SourceGenerators/Configuration/ConfigKeyAliasesSwitcherGenerator.cs:24:0-340:1) to use the shared constant - Added documentation as to how to add a key now ## Reason for change The [configuration_keys_mapping.json](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/configuration_keys_mapping.json:0:0-0:0) file was being ignored during code generation, causing the generator to produce incorrect constant names. This meant that any manual edits to constant names in the mapping file were not being respected, and the generated code would use auto-generated names instead of the explicitly mapped ones. Additionally, the file header comments explaining that files are auto-generated were duplicated across generators, violating DRY principles. ## Implementation details 1. **Fixed array extraction logic**: Replaced the call to [JsonReader.ExtractJsonObjectSection()](cci:1://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace.SourceGenerators/Configuration/JsonReader.cs:16:4-91:5) with custom array extraction code that: - Finds the `"mappings":` key in the JSON - Locates the opening `[` bracket - Tracks bracket nesting to find the matching closing `]` - Extracts the complete array content 2. **Resolved variable scope issue**: Reused existing `inString` and `escapeNext` variables from the outer scope instead of redeclaring them, fixing compilation errors. 3. **Centralized header comments**: Created `Constants.ConfigurationGeneratorComment` containing the standardized auto-generation notice and updated both generators to use it. ## Test coverage - Verified the generator builds successfully without errors - The mapping file is now properly parsed and applied during code generation - Generated constant names now match the mappings defined in [configuration_keys_mapping.json](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/configuration_keys_mapping.json:0:0-0:0) ## Other details This fix ensures that the explicit naming conventions defined in [configuration_keys_mapping.json](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/configuration_keys_mapping.json:0:0-0:0) are respected, maintaining consistency with the existing codebase and preventing future confusion when constant names don't match their expected values. --------- Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>
70df7c9 to
d197570
Compare
c8caa83 to
85c37d3
Compare
d197570 to
272e31a
Compare
85c37d3 to
bea964f
Compare
272e31a to
c89c676
Compare
bea964f to
d62b438
Compare
c89c676 to
41c88a5
Compare
d62b438 to
ae8195d
Compare
41c88a5 to
e091036
Compare
ae8195d to
2a87981
Compare
e091036 to
b7ac294
Compare
2a87981 to
c14af6e
Compare
b7ac294 to
f164b06
Compare
c14af6e to
99e80f7
Compare
f164b06 to
756c6b3
Compare
99e80f7 to
0d9d4be
Compare
756c6b3 to
60360fa
Compare
8461312 to
be89b22
Compare
9227c7e to
a1ceb97
Compare
353984d to
de53524
Compare
0af9145 to
d16d6c6
Compare
26977ff to
db36d61
Compare
d16d6c6 to
ff1db4e
Compare
db36d61 to
46a9848
Compare
ff1db4e to
1bb6dc6
Compare
andrewlock
left a comment
There was a problem hiding this comment.
Looks good in principal, but I think we need to make some tweaks to the generated code
| context.AddSource(enumToGenerate.ExtensionsName + "_EnumExtensions.g.cs", SourceText.From(result, Encoding.UTF8)); | ||
|
|
||
| // If this is the IntegrationId enum, also generate IntegrationNameToKeys | ||
| if (enumToGenerate.FullyQualifiedName == "Datadog.Trace.Configuration.IntegrationId") |
There was a problem hiding this comment.
This feels so hacky, but I love it 😂
| sb.AppendLine(Constants.FileHeader); | ||
| sb.AppendLine("namespace Datadog.Trace.Configuration"); | ||
| sb.AppendLine("{"); | ||
| sb.AppendLine(" /// <summary>"); |
There was a problem hiding this comment.
It looks neat, but doing these as individual sb.AppendLine is very expensive. You want to do as big a block as you can in general, so raw strings are the best option generally.
e.g.
sb.AppendLine(
Constants.FileHeader + // as long as this is a constant, the concat happens at compile time
"""
namespace Datadog.Trace.Configuration
{
/// Generated mapping of integration names to their configuration keys.");
/// </summary>");
internal static partial class IntegrationNameToKeys");
{
private const string ObsoleteMessage = DeprecationMessages.AppAnalytics;");
/// <summary>");
/// All integration enabled keys (canonical + aliases).");
/// </summary>");
public static readonly string[] AllIntegrationEnabledKeys = new[]");
{
""");etc
It also means you don't need to escape anything, just copy-paste the code you expect in there, and "break out" where you need something specific (like the actual values) 🙂
| // Single loop to build array keys and all switch cases | ||
| foreach (var member in enumToGenerate.Names) |
There was a problem hiding this comment.
nit, I would have just iterated it 3 times, and append to the same stringbuilder instead of having multiple builders. The iteration is low overhead compared to the stringbuilder (which will each have to grow their internal array multiple times)
| sb.AppendLine(" {"); | ||
| sb.AppendLine(" string.Format(\"DD_TRACE_{0}_ENABLED\", integrationName.ToUpperInvariant()),"); | ||
| sb.AppendLine(" string.Format(\"DD_TRACE_{0}_ENABLED\", integrationName),"); | ||
| sb.AppendLine(" $\"DD_{integrationName}_ENABLED\""); | ||
| sb.AppendLine(" }"); |
There was a problem hiding this comment.
This is an interesting one tbh, do we even need this branch 🤔 Given we handle this at compile time, we know it will never be called. That said, throwing probably isn't great.
We might want to generate something more like this:
_ => GetEnabledFallbackFor(integrationName),because it should generate smaller code, which has more chance of optimization, and the fallback will never be invoked,
| _source, | ||
| _telemetry, | ||
| integrationEnabledKeys[0], | ||
| integrationEnabledKeys.Skip(1).ToArray()); |
There was a problem hiding this comment.
oh no, Skip(1).ToArray() 🙁 We definitely don't want to do that, If we're going to need to do that, we should change the GetIntegrationEnabledKeys() API to return an array of the correct size (e.g. a "main" key plus an array of fallbacks).
If possible we should probably try to get away from using arrays at all... we may be able to use ReadOnlySpan<string> in some places, or we should use InlineArray (if we can), or worse case we should create a specialized type to avoid creating all these small arrays if possible.
| { | ||
| return integrationName switch | ||
| { | ||
| "HttpMessageHandler" => new[] { "DD_TRACE_HTTPMESSAGEHANDLER_ENABLED", "DD_TRACE_HttpMessageHandler_ENABLED", "DD_HttpMessageHandler_ENABLED" }, |
There was a problem hiding this comment.
We should use collection expressions wherever possible
| "HttpMessageHandler" => new[] { "DD_TRACE_HTTPMESSAGEHANDLER_ENABLED", "DD_TRACE_HttpMessageHandler_ENABLED", "DD_HttpMessageHandler_ENABLED" }, | |
| "HttpMessageHandler" => ["DD_TRACE_HTTPMESSAGEHANDLER_ENABLED", "DD_TRACE_HttpMessageHandler_ENABLED", "DD_HttpMessageHandler_ENABLED"], |
| /// <summary> | ||
| /// All integration enabled keys (canonical + aliases). | ||
| /// </summary> | ||
| public static readonly string[] AllIntegrationEnabledKeys = new[] |
There was a problem hiding this comment.
AFAICT, this is only called in a test. But the array will be created as soon as we call IntegrationNameToKeys.Something. We can solve that by making it a method instead (and also marking it as only to be called in tests)
| public static readonly string[] AllIntegrationEnabledKeys = new[] | |
| [TestingOnly] | |
| public static readonly string[] GetAllIntegrationEnabledKeys() => new[] |
472f9f9 to
57ec51a
Compare
b481629 to
d8e691c
Compare
57ec51a to
d1c5ab9
Compare
d8e691c to
cafe31e
Compare
d1c5ab9 to
d15b826
Compare
…apt everywhere (#7932) ## Context Part of **Configuration Inversion (Step 6)** - Stack progress: 1. [#7548](#7548) - Add GitLab step and JSON configuration file 2. [#7688](#7688) - Cleanup configuration / platform keys + source generator 3. [#7698](#7698) - Aliases handling via source generator 4. [#7689](#7689) - Analyzers for platform and ConfigurationBuilder 5. [#7931](#7931) - Replace manual ConfigurationKeys by generated ones in the whole solution 6. **→ [#7932](#7932) - Forbid use of System.Environment methods and adapt everywhere (this PR)** 7. [#7937](#7937) - Integration names to generated keys I'll update the PR summary to mention the YAML documentation file: ## Summary of changes Banned direct `System.Environment.GetEnvironmentVariable()` usage and migrated all environment variable access to use `EnvironmentHelpers` with strongly-typed [ConfigurationKeys](cci:2://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Generated/net461/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.g.cs:15:0-737:1) and `PlatformKeys` constants. ### Key changes: - **Banned `System.Environment.GetEnvironmentVariable()`** via `BannedApiAnalyzers` - **Added `EnvironmentGetEnvironmentVariableAnalyzer` (DD0009)** to enforce [ConfigurationKeys](cci:2://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Generated/net461/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.g.cs:15:0-737:1)/`PlatformKeys` usage only - **Nested `PlatformKeys` by category** (Ci, Aws, AzureAppService, ServiceFabric, DotNet) - **Migrated ~50+ files** across CI Visibility, AWS Lambda, AppSec, Telemetry, Profiler, and Agent components - **Added missing keys** to [supported-configurations.json](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/supported-configurations.json:0:0-0:0) and their documentation to [supported-configurations-docs.yaml](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/supported-configurations-docs.yaml:0:0-0:0) ## Reason for change Direct `System.Environment.GetEnvironmentVariable()` calls with string literals are error-prone. Centralizing through `EnvironmentHelpers` with strongly-typed constants provides compile-time validation, discoverability, and refactoring safety. ## Implementation details 1. **Banned API enforcement** - Added `BannedSymbols.txt` and configured `.editorconfig` to treat RS0030 as error (vendored code excluded) 2. **Custom analyzer** - DD0009 validates all `EnvironmentHelpers` calls accept only [ConfigurationKeys](cci:2://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Generated/net461/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.g.cs:15:0-737:1)/`PlatformKeys` constants, rejecting hardcoded strings 3. **PlatformKeys organization** - Nested by category for better discoverability 4. **EnvironmentHelpers refactoring** - Added overloads for nested class constants, maintained backward compatibility 5. **Configuration documentation** - Added missing configuration keys to [supported-configurations.json](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/supported-configurations.json:0:0-0:0) and comprehensive XML documentation to [supported-configurations-docs.yaml](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/supported-configurations-docs.yaml:0:0-0:0), which the source generator uses to generate XML doc comments in [ConfigurationKeys](cci:2://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Generated/net461/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.g.cs:15:0-737:1) classes ## Test coverage - ✅ Comprehensive analyzer tests covering valid/invalid scenarios - ✅ All existing unit tests pass - ✅ Banned API analyzer correctly flags direct `System.Environment` usage ## Other details +2,600/-940 lines. No breaking changes, negligible performance impact.
d15b826 to
884fe46
Compare
|
Removing @DataDog/serverless-azure-and-gcp as a reviewer since I don't see any Azure or GCP related changes. Maybe there were some changes before a force push but I don't see any now. |
f03a983 to
46c691d
Compare
BenchmarksBenchmark execution time: 2026-01-27 10:41:58 Comparing candidate commit 72c8d09 in PR branch Found 5 performance improvements and 9 performance regressions! Performance is the same for 164 metrics, 14 unstable metrics. scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs netcoreapp3.1
scenario:Benchmarks.Trace.AspNetCoreBenchmark.SendRequest net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OriginalCharSlice net6.0
scenario:Benchmarks.Trace.ILoggerBenchmark.EnrichedLog net6.0
scenario:Benchmarks.Trace.Log4netBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.SingleSpanAspNetCoreBenchmark.SingleSpanAspNetCore netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishSpan net6.0
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishTwoScopes net6.0
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishTwoScopes netcoreapp3.1
|
Context
Part of Configuration Inversion (Step 6) - Stack progress:
Summary of changes
Extends the
EnumExtensionsGeneratorto generate IntegrationNameToKeys class for theIntegrationIdenum, providing centralized configuration key mapping for integrations including enabled, analytics enabled, and analytics sample rate keys. Also fixes AdoNet instrumentation compilation error.Reason for change
Implementation details
EnumExtensionsGeneratorwith GenerateIntegrationNameToKeys method that generates:AllIntegrationEnabledKeysarray containing all enabled configuration keysIntegrationEnabledKeyPattern,AnalyticsEnabledKeyPattern,AnalyticsSampleRateKeyPattern)Test coverage
IntegrationIdenum