[Config] Register all native configuration variables (3/3)#8733
Conversation
This comment has been minimized.
This comment has been minimized.
a74d141 to
2fe83b1
Compare
70f1b76 to
867e7b9
Compare
2fe83b1 to
ace5613
Compare
867e7b9 to
843253b
Compare
BenchmarksBenchmark execution time: 2026-06-17 14:39:19 Comparing candidate commit f7a56ec in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 63 known flaky benchmarks, 63 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8733) 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 (8733) - mean (74ms) : 70, 77
master - mean (73ms) : 71, 75
section Bailout
This PR (8733) - mean (79ms) : 76, 82
master - mean (79ms) : 75, 82
section CallTarget+Inlining+NGEN
This PR (8733) - mean (1,100ms) : 1046, 1153
master - mean (1,093ms) : 1051, 1135
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 (8733) - mean (117ms) : 112, 122
master - mean (115ms) : 109, 122
section Bailout
This PR (8733) - mean (115ms) : 111, 120
master - mean (117ms) : 112, 122
section CallTarget+Inlining+NGEN
This PR (8733) - mean (792ms) : 763, 821
master - mean (788ms) : 769, 808
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8733) - mean (102ms) : 98, 106
master - mean (104ms) : 99, 108
section Bailout
This PR (8733) - mean (105ms) : 101, 109
master - mean (103ms) : 100, 107
section CallTarget+Inlining+NGEN
This PR (8733) - mean (951ms) : 904, 999
master - mean (953ms) : 916, 990
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8733) - mean (99ms) : 95, 102
master - mean (99ms) : 96, 103
section Bailout
This PR (8733) - mean (100ms) : 98, 102
master - mean (103ms) : 99, 106
section CallTarget+Inlining+NGEN
This PR (8733) - mean (824ms) : 791, 856
master - mean (819ms) : 778, 861
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 (8733) - mean (201ms) : 195, 206
master - mean (199ms) : 193, 206
section Bailout
This PR (8733) - mean (204ms) : 200, 209
master - mean (204ms) : 199, 208
section CallTarget+Inlining+NGEN
This PR (8733) - mean (1,200ms) : 1165, 1236
master - mean (1,192ms) : 1156, 1228
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 (8733) - mean (287ms) : 280, 294
master - mean (284ms) : 277, 291
section Bailout
This PR (8733) - mean (287ms) : 281, 293
master - mean (289ms) : 284, 294
section CallTarget+Inlining+NGEN
This PR (8733) - mean (966ms) : 945, 987
master - mean (966ms) : 948, 985
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8733) - mean (282ms) : 276, 288
master - mean (278ms) : 272, 285
section Bailout
This PR (8733) - mean (280ms) : 274, 286
master - mean (277ms) : 269, 285
section CallTarget+Inlining+NGEN
This PR (8733) - mean (1,163ms) : 1121, 1205
master - mean (1,157ms) : 1119, 1196
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8733) - mean (279ms) : 271, 288
master - mean (276ms) : 268, 285
section Bailout
This PR (8733) - mean (278ms) : 272, 285
master - mean (277ms) : 273, 281
section CallTarget+Inlining+NGEN
This PR (8733) - mean (1,039ms) : 1003, 1075
master - mean (1,035ms) : 995, 1076
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
773fe74 to
961610d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 961610dc30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Logger.Information("Native configuration validation passed: all {Count} native DD_* variables are registered with native scope", nativeVars.Count); | ||
| } | ||
|
|
||
| private static Dictionary<string, RegistryEntry> ParseRegistry(AbsolutePath supportedConfigurationsPath) |
There was a problem hiding this comment.
Given this parsing should stay in sync with the source generator, should/could we just source link it in, to avoid the duplication. I feel like it could easily drift otherwise 🤔
## Summary of changes Adds `scope: managed` to every existing entry in `supported-configurations.yaml`. Also adds `scope` to the YAML parser's recognized property list so the field is accepted without throwing. This is the first of three stacked PRs introducing scope tracking to the configuration registry: - **PR 1 (this)**: Add `scope: managed` to all existing entries — pure data + one-line parser guard - [**PR 2**](#8732): Full scope infrastructure — parsing, generator enforcement, tests, docs - [**PR 3**](#8733): All native config variables — new `scope: native` entries + `scope: managed, native` upgrades ## Reason for change Prerequisite for scope-aware config registry. The parser must recognize the field before the data lands; the generator infrastructure lands in PR 2. ## Implementation details - `YamlReader.cs`: one-line addition — `"scope"` added to the recognized property list (prevents `InvalidOperationException` on unknown fields) - `supported-configurations.yaml`: `scope: managed` inserted after every `- implementation:` line across all 317 existing entries No behavioral change — the generator does not yet read or act on the scope field. ## Test coverage Existing test suite passes unchanged. The generator ignores the scope field at this point so no new tests are needed here. ## Other details Part of a larger effort to register all native-consumed `DD_*` variables in the config registry and enforce coverage via a CI test. Co-authored-by: Claude Sonnet 4.6 <[email protected]>
a5b94f6 to
d823642
Compare
360413f to
bc96043
Compare
bc96043 to
7103075
Compare
anna-git
left a comment
There was a problem hiding this comment.
LGTM, just left a last comment on comparisons
Also there's 2 vars showing up on CI with the wrong versions
andrewlock
left a comment
There was a problem hiding this comment.
LGTM in general, but can we not use a dedicated project for this one validation, and instead make it a nuke target like other validation workflows?
| - name: "Validate native configurations are registered" | ||
| run: dotnet run -c Release --project tracer/src/Datadog.Trace.Tools.NativeConfigValidator/Datadog.Trace.Tools.NativeConfigValidator.csproj -- "${{ github.workspace }}" |
There was a problem hiding this comment.
The standard way to do this is to use a Nuke target, like we do for other validation workflows - can we keep that approach for consistency?
There was a problem hiding this comment.
Good call - done in 46689e3, workflow now runs ./tracer/build.sh ValidateNativeConfigurations.
One thing: the target wraps the standalone NativeConfigValidator tool instead of inlining like CreateMissingNullabilityFile, because source-linking YamlReader into _build breaks the build_* jobs with CS2001 (Docker only COPYs _build). wdyt, or did you want it inline?
There was a problem hiding this comment.
I don't think we should have a standalone NativeConfigValidator... you could fix the source linking issue by reversing the copy. i.e. move the file to nuke, and source link into the source generator instead of the other way around
There was a problem hiding this comment.
Ah, I get it now...
Updated in 7a4dce8 !
## Summary of changes Adds `scope: managed` to every existing entry in `supported-configurations.yaml`. Also adds `scope` to the YAML parser's recognized property list so the field is accepted without throwing. This is the first of three stacked PRs introducing scope tracking to the configuration registry: - **PR 1 (this)**: Add `scope: managed` to all existing entries — pure data + one-line parser guard - [**PR 2**](#8732): Full scope infrastructure — parsing, generator enforcement, tests, docs - [**PR 3**](#8733): All native config variables — new `scope: native` entries + `scope: managed, native` upgrades ## Reason for change Prerequisite for scope-aware config registry. The parser must recognize the field before the data lands; the generator infrastructure lands in PR 2. ## Implementation details - `YamlReader.cs`: one-line addition — `"scope"` added to the recognized property list (prevents `InvalidOperationException` on unknown fields) - `supported-configurations.yaml`: `scope: managed` inserted after every `- implementation:` line across all 317 existing entries No behavioral change — the generator does not yet read or act on the scope field. ## Test coverage Existing test suite passes unchanged. The generator ignores the scope field at this point so no new tests are needed here. ## Other details Part of a larger effort to register all native-consumed `DD_*` variables in the config registry and enforce coverage via a CI test. Co-authored-by: Claude Sonnet 4.6 <[email protected]>
Adds 86 native-only configuration variables to supported-configurations.yaml
with scope: native, covering the tracer, shared loader, profiler, and
DD_INTERNAL_* components. Upgrades 25 entries from scope: managed to
scope: managed, native where native code also reads the variable.
Adds NativeEnvVarCoverageTests.cs, which verifies that every DD_*
variable read by WStr("DD_...") or L"DD_..." in native C++ source
(tracer/src, shared/src, profiler/src) has a corresponding YAML entry
with native in its scope.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
DD_IAST_SECURITY_CONTROLS_CONFIGURATION, DD_PROFILING_EXCEPTION_ENABLED, DD_PROFILING_WALLTIME_ENABLED, and DD_TRACE_AZURE_FUNCTIONS_ENABLED all exist in the Feature Parity Dashboard under implementation B, not A. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…ENABLED Both default to true in the profiler source (confirmed in Configuration.cpp). Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…ries Verified all 86 new native-only entries against the C++ source files. Corrections applied: Defaults (26 entries): various profiler tuning thresholds had null where the C++ source has explicit fallback values; several booleans had false where the source defaults to true (ETW enabled, timestamps as label, GC threads CPU time, thread lifetime, lock contention, exception profiling, etc.) Types (1 entry): DD_INTERNAL_CIVISIBILITY_SPANID is an integer (uint64), not string. Products (3 entries): DD_PROFILER_PROCESSES and DD_PROFILER_EXCLUDE_PROCESSES are read by the CLR auto-instrumentation profiler, not the Continuous Profiler — product changed to Tracer. DD_HOSTNAME is read exclusively by the profiler engine — product changed to Profiler. Documentation (12 entries): corrected units (milliseconds vs minutes), clarified that DD_INTERNAL_CUSTOM_CLR_PROFILER_PATH and DD_INTERNAL_NATIVE_LOADER_PATH are output variables set by the loader (not user overrides), fixed DD_DUMP_ILREWRITE_ENABLED to say "log" not "disk", fixed DD_INTERNAL_WORKAROUND_77973_ENABLED to describe tiered compilation (not IL verification), and several others. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Dictionary.TryAdd is not available on .NET Framework 4.8 / netstandard2.0, which the SourceGenerators.Tests project targets. The local build on macOS skips net48 so this only surfaced on the Windows CI runner. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Per review feedback: validating the native config registry from a managed source-generator unit test was the wrong altitude — source-generator tests should test generator behavior, and developers working only in the native solutions would never run it locally. - Adds NativeConfigValidator (tracer/build/_build/NativeValidation) and a ValidateNativeConfigurations Nuke target. It scans tracer/src/Datadog.Tracer.Native, shared/src, and profiler/src for DD_*/_DD_* env var literals and validates each is registered with native scope. - Uses a plain string-literal regex (catches reads the previous WStr/L"..." patterns missed, e.g. MetadataProvider.cpp), includes the _DD_ prefix, excludes crashhandler.cpp (captures all DD vars at crash time), and allowlists two non-config literals (DD_ETW_DISPATCHER named pipe, DD_ETW_IPC_V1 magic version). - Shares YamlReader.cs with the source generator via a csproj <Compile Include> link. - Wires the target into the verify_source_generators GitHub workflow. - Removes NativeEnvVarCoverageTests.cs from the managed test project. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
The native config scanner excluded .c sources, so the Linux ApiWrapper (functions_to_wrap.c) was skipped - it reads DD_INTERNAL_CRASHTRACKING_PASSTHROUGH and DD_INTERNAL_CRASHTRACKING_MINIDUMPNAME, neither of which was registered. Adds .c (and .cxx) to the scanned extensions and registers both vars with scope: native. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Every other DD_TELEMETRY_* entry uses product: Telemetry; this was the lone Tracer outlier. No functional effect (native-only entries generate no constant), but keeps the product grouping consistent with the prefix. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…ss projects The csproj <Compile Include> linked YamlReader.cs from the source generators project via a ..\..\src path. That broke the build CI Docker images, whose Dockerfiles COPY only the _build directory in isolation and pre-build it - the cross-directory source file isn't in that context, so dotnet build failed with CS2001 (and took down every build_* job). NativeConfigValidator now parses supported-configurations.yaml with YamlDotNet, which the build project already references. No cross-project file link, no CS8632 suppression, and the validator no longer depends on the managed source generators. Verified by compiling the _build project in an isolated copy (mirroring the Docker COPY context). Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…own action Address andrewlock review feedback on the native-config validator: - Relocate the validator out of the Nuke _build project into a standalone Datadog.Trace.Tools.NativeConfigValidator tool, run by a dedicated GitHub Action. A failure there means "update the registry" — distinct from the source-generator verification job, so it gets its own workflow. - Reuse the source generator's YamlReader (source-linked) instead of a duplicate YamlDotNet parser, so the validator and the ConfigurationKeys generator can never drift out of sync. - Drop the restriction that native-only entries cannot declare a const_name (harmless, and leaves room to source-generate native constants later). - Use default (ordinal) comparers and include the registry file path in the "missing variable" error. - Exclude the tool from CompileManagedSrc; like Tools.Runner it is built and restored only by its own workflow, not the main managed build. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The documentation prose contradicted the structured `default:` field (which matches the native code default) for these native-only keys: - DD_INTERNAL_FAULT_TOLERANT_INSTRUMENTATION_ENABLED (default is false) - DD_INTERNAL_SKIP_METHOD_BODY_ENABLED (default is true) - DD_INTERNAL_TRACE_VERSION_COMPATIBILITY (default is true) Correct the prose to match the runtime default. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…figValidator.cs Co-authored-by: Anna <[email protected]>
Per andrewlock review feedback (r3412835267): run the native-config validation through a Nuke target like the other validation workflows, instead of `dotnet run` directly. - Re-add the ValidateNativeConfigurations Nuke target, as a thin wrapper that runs the standalone Datadog.Trace.Tools.NativeConfigValidator tool. The logic stays in the tool (not inline in _build) because source-linking the generator's YamlReader into _build hits CS2001 in the isolated Docker build context - the reason it was moved out in the first place. - Point validate_native_configurations.yml at `./tracer/build.sh ValidateNativeConfigurations`, keeping its own dedicated workflow. - Expand the workflow path filter to include tracer/build.sh and tracer/build/_build/** so changes to the target or build entrypoint also trigger the gate. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Shorten the ValidateNativeConfigurations target comment to ~3 lines (in line with the build files' norm) and drop the redundant path-filter comment. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…t during CI incident) Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Per andrewlock review feedback: remove the standalone Datadog.Trace.Tools.NativeConfigValidator tool and run validation inline in the Nuke build, fixing the CS2001 source-link issue by reversing the copy direction. - Move YamlReader.cs + EquatableArray.cs into tracer/build/_build/NativeValidation/ (kept namespace Datadog.Trace.SourceGenerators.Helpers). _build has the restrictive Docker build context (COPYs only _build), so the shared parser must live there; the source generator builds from a full checkout and now source-links them *from* _build (the reverse direction is what failed CS2001). - Add _build/NativeValidation/NativeConfigValidator.cs and run it inline from the ValidateNativeConfigurations Nuke target (throws on failure). - Delete the standalone tool project and its CompileManagedSrc exclude; drop its now-dead path from the workflow filter. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Rebased onto master; the validator (running on the merge commit) flagged 3 DD_INTERNAL_PROFILING_HEAPSNAPSHOT_* / TEST_HEAPSNAPSHOT vars that landed on master with the heap-snapshot feature but were never registered. Add them with the metadata read from Configuration.cpp: - ..._HEAPSNAPSHOT_REFERENCE_TREE_FORMAT: int, default 1 (binary bitfield) - ..._HEAPSNAPSHOT_SKIP_TRAVERSAL: boolean, default false - ..._TEST_HEAPSNAPSHOT_INTERVAL: int seconds, default 0 (test-only) Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
0e5436b to
f7a56ec
Compare
andrewlock
left a comment
There was a problem hiding this comment.
LGTM, thanks for the iteration on this!
Summary of changes
Registers previously unregistered native-only
DD_*configuration variables and upgrades existing entries that are also read by native code toscope: managed, native. Adds a standalone validator tool, run by a dedicated GitHub Action, that enforces complete native coverage.Stack:
scope: managedon all existing entriesReason for change
Many native-consumed
DD_*variables were absent fromsupported-configurations.yaml, making the registry an incomplete picture of the tracer's configuration surface. The validator added here fails CI if a new native variable is read without a registry entry.Implementation details
Registry entries — native-only keys get
scope: native; keys read by both native and managed code are upgraded toscope: managed, native(DD_TRACE_ENABLED,DD_ENV,DD_SERVICE,DD_APPSEC_ENABLED,DD_INJECTION_ENABLED, etc.). Coverage spans tracer native (DD_CLR_*,DD_LOADER_*), the shared loader (DD_CRASHTRACKING_*,DD_TELEMETRY_FORWARDER_PATH),DD_INTERNAL_*, and the Continuous Profiler (DD_PROFILING_*).Native coverage validator —
tracer/src/Datadog.Trace.Tools.NativeConfigValidator, a standalone tool run by the dedicatedvalidate_native_configurationsGitHub Action. It scanstracer/src/Datadog.Tracer.Native,shared/src, andprofiler/srcforDD_*/_DD_*literals and asserts each is registered withnativescope. It reuses the source generator'sYamlReader(source-linked) so the validator and theConfigurationKeysgenerator cannot drift. A failure means "update the registry", so it lives in its own workflow rather than the source-generator verification job.Test coverage
The validator runs in its own GitHub Action on PRs touching the registry or native source, and passes against the current registry (123 native variables covered).
Other details
The
validate_supported_configurations_v2_local_fileCI check requires every entry in this file to be registered in the central Configuration Registry. The new native-only entries are not yet there. I will register them before merging.