-
Notifications
You must be signed in to change notification settings - Fork 149
[Config Registry] 6/8 Forbid use of System.Environment methods and adapt everywhere #7932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
95b0301 to
39a7db5
Compare
346d13a to
1440fdf
Compare
39a7db5 to
7b7e9bf
Compare
1440fdf to
dad2367
Compare
…any string key (#7689) ## Context Part of **Configuration Inversion (Step 4)** - Stack progress: 1. [#7548](#7548) - Add GitLab step and JSON configuration file 2. [#7688](#7688) - Cleanup configuration / platform keys + analyzers 3. [#7698](#7698) - Source generator for ConfigurationKeys 4. **→ [#7689](#7689) - Aliases handling and analyzers (this PR)** 5. [#7931](#7931) - Replace manual ConfigurationKeys with generated version 6. #[7932](#7932 Forbid use of System.Environment methods and adapt everywhere ## Summary Adds source generator for configuration key aliases, integrates alias resolution into `ConfigurationBuilder`, and adds Roslyn analyzers to enforce proper configuration key usage. ## Changes **Alias Source Generator:** - `ConfigKeyAliasesSwitcherGenerator` reads aliases from `supported-configurations.json` - Auto-generates switch statements to resolve primary keys from aliases - Generated for all target frameworks (net461, netstandard2.0, netcoreapp3.1, net6.0) **ConfigurationBuilder Improvements:** - Integrated alias resolution directly into `ConfigurationBuilder.WithKeys()` - Removed manual fallback overloads throughout codebase - Added `GetKeyWithAlias()` method to `IConfigurationSource` interface **IntegrationSettings Special Handling:** - Updated to use pattern-based key construction (e.g., `DD_TRACE_{INTEGRATION}_ENABLED`) - Simplified configuration reading by leveraging alias system **Roslyn Analyzers:** - **DD0007**: `PlatformKeysAnalyzer` - Enforces use of `PlatformKeys` for external platform environment variables - **DD0008**: `ConfigurationBuilderWithKeysAnalyzer` - Enforces use of [ConfigurationKeys](cci:2://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/ConfigurationKeys.Logging.cs:9:0-58:1)/`PlatformKeys` constants in `ConfigurationBuilder.WithKeys()` calls **Special Cases:** - Fixed `DatadogLoggingFactory` to handle deprecated `DD_TRACE_LOG_PATH` with proper analyzer suppression - Updated configuration tests to work with alias resolution ## Motivation Completes configuration inversion by: - Auto-generating alias resolution from `supported-configurations.json` - Eliminating manual fallback chains - Enforcing compile-time validation of configuration keys - Preventing hardcoded strings and typos ## Test Coverage - Added `ConfigKeyAliasesSwitcherGeneratorTests` with comprehensive test coverage - Added `PlatformKeysAnalyzerTests` with 146 lines of tests - Added `ConfigurationBuilderWithKeysAnalyzerTests` with 538 lines of tests - Updated existing configuration tests for alias support - All tests pass with new alias system ## Related Work Builds on #7548 (configuration registry), #7688 (PlatformKeys separation), and #7698 (source generator for ConfigurationKeys).
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7932) 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 (7932) - mean (69ms) : 67, 70
master - mean (68ms) : 67, 70
section Bailout
This PR (7932) - mean (72ms) : 71, 74
master - mean (72ms) : 71, 73
section CallTarget+Inlining+NGEN
This PR (7932) - mean (1,017ms) : 951, 1083
master - mean (1,007ms) : 967, 1047
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 (7932) - mean (106ms) : 104, 108
master - mean (106ms) : 103, 108
section Bailout
This PR (7932) - mean (107ms) : 105, 108
master - mean (107ms) : 106, 108
section CallTarget+Inlining+NGEN
This PR (7932) - mean (735ms) : 663, 806
master - mean (735ms) : 672, 799
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7932) - mean (93ms) : 91, 95
master - mean (94ms) : 92, 96
section Bailout
This PR (7932) - mean (94ms) : 93, 95
master - mean (94ms) : 93, 95
section CallTarget+Inlining+NGEN
This PR (7932) - mean (711ms) : 671, 750
master - mean (709ms) : 672, 747
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7932) - mean (92ms) : 90, 95
master - mean (92ms) : 90, 94
section Bailout
This PR (7932) - mean (93ms) : 92, 94
master - mean (93ms) : 92, 95
section CallTarget+Inlining+NGEN
This PR (7932) - mean (634ms) : 618, 650
master - mean (633ms) : 616, 650
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 (7932) - mean (193ms) : 189, 198
master - mean (193ms) : 189, 197
section Bailout
This PR (7932) - mean (197ms) : 193, 200
master - mean (197ms) : 192, 201
section CallTarget+Inlining+NGEN
This PR (7932) - mean (1,117ms) : 1058, 1176
master - mean (1,113ms) : 1058, 1169
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 (7932) - mean (276ms) : 272, 280
master - mean (277ms) : 271, 282
section Bailout
This PR (7932) - mean (277ms) : 273, 281
master - mean (276ms) : 273, 279
section CallTarget+Inlining+NGEN
This PR (7932) - mean (927ms) : 877, 976
master - mean (922ms) : 871, 974
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7932) - mean (270ms) : 266, 274
master - mean (270ms) : 265, 275
section Bailout
This PR (7932) - mean (270ms) : 267, 273
master - mean (269ms) : 264, 274
section CallTarget+Inlining+NGEN
This PR (7932) - mean (922ms) : 873, 971
master - mean (923ms) : 874, 972
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7932) - mean (268ms) : 264, 273
master - mean (269ms) : 264, 274
section Bailout
This PR (7932) - mean (269ms) : 264, 274
master - mean (269ms) : 265, 272
section CallTarget+Inlining+NGEN
This PR (7932) - mean (822ms) : 803, 841
master - mean (822ms) : 800, 845
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
7b7e9bf to
8ba62d5
Compare
dad2367 to
f18f8e4
Compare
303b602 to
70df7c9
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 <[email protected]>
c89c676 to
41c88a5
Compare
f164b06 to
756c6b3
Compare
GreenMatan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed debugger related code change
60360fa to
6e67a5d
Compare
…ent are called with ConfigurationKeys/PlatformKeys
6e67a5d to
7e8b0c6
Compare
Context
Part of Configuration Inversion (Step 6) - Stack progress:
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 useEnvironmentHelperswith strongly-typed ConfigurationKeys andPlatformKeysconstants.Key changes:
System.Environment.GetEnvironmentVariable()viaBannedApiAnalyzersEnvironmentGetEnvironmentVariableAnalyzer(DD0009) to enforce ConfigurationKeys/PlatformKeysusage onlyPlatformKeysby category (Ci, Aws, AzureAppService, ServiceFabric, DotNet)Reason for change
Direct
System.Environment.GetEnvironmentVariable()calls with string literals are error-prone. Centralizing throughEnvironmentHelperswith strongly-typed constants provides compile-time validation, discoverability, and refactoring safety.Implementation details
BannedSymbols.txtand configured.editorconfigto treat RS0030 as error (vendored code excluded)EnvironmentHelperscalls accept only ConfigurationKeys/PlatformKeysconstants, rejecting hardcoded stringsTest coverage
System.EnvironmentusageOther details
+2,600/-940 lines. No breaking changes, negligible performance impact.