[Config] Add scope field infrastructure to configuration registry (2/3)#8732
Merged
Conversation
bm1549
added
the
AI Generated
This comment has been minimized.
This comment has been minimized.
bm1549
force-pushed
the
brian.marks/native-config-scope-pr2
branch
from
June 2, 2026 17:22
a74d141 to
2fe83b1
Compare
bm1549
force-pushed
the
brian.marks/native-config-scope-pr1
branch
from
June 2, 2026 17:24
e8fba4e to
9fb89ee
Compare
bm1549
force-pushed
the
brian.marks/native-config-scope-pr2
branch
from
June 2, 2026 17:24
2fe83b1 to
ace5613
Compare
BenchmarksBenchmark execution time: 2026-06-10 16:56:59 Comparing candidate commit e3731a8 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 70 metrics, 0 unstable metrics, 62 known flaky benchmarks, 64 flaky benchmarks without significant changes.
|
anna-git
reviewed
Jun 3, 2026
anna-git
reviewed
Jun 3, 2026
anna-git
reviewed
Jun 3, 2026
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8732) 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 (8732) - mean (73ms) : 69, 77
master - mean (74ms) : 69, 78
section Bailout
This PR (8732) - mean (77ms) : 75, 79
master - mean (77ms) : 75, 79
section CallTarget+Inlining+NGEN
This PR (8732) - mean (1,097ms) : 1044, 1149
master - mean (1,100ms) : 1048, 1151
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 (8732) - mean (114ms) : 107, 120
master - mean (115ms) : 107, 123
section Bailout
This PR (8732) - mean (113ms) : 111, 116
master - mean (114ms) : 111, 117
section CallTarget+Inlining+NGEN
This PR (8732) - mean (795ms) : 767, 823
master - mean (800ms) : 778, 822
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8732) - mean (101ms) : 96, 106
master - mean (100ms) : 97, 103
section Bailout
This PR (8732) - mean (105ms) : 101, 109
master - mean (100ms) : 99, 102
section CallTarget+Inlining+NGEN
This PR (8732) - mean (944ms) : 913, 976
master - mean (950ms) : 910, 990
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8732) - mean (98ms) : 94, 102
master - mean (102ms) : 96, 107
section Bailout
This PR (8732) - mean (99ms) : 97, 102
master - mean (101ms) : 98, 103
section CallTarget+Inlining+NGEN
This PR (8732) - mean (827ms) : 787, 866
master - mean (824ms) : 788, 860
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 (8732) - mean (198ms) : 193, 204
master - mean (198ms) : 194, 202
section Bailout
This PR (8732) - mean (201ms) : 196, 206
master - mean (201ms) : 198, 204
section CallTarget+Inlining+NGEN
This PR (8732) - mean (1,189ms) : 1161, 1218
master - mean (1,203ms) : 1141, 1265
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 (8732) - mean (281ms) : 275, 288
master - mean (284ms) : 278, 290
section Bailout
This PR (8732) - mean (281ms) : 274, 288
master - mean (285ms) : 279, 291
section CallTarget+Inlining+NGEN
This PR (8732) - mean (955ms) : 939, 972
master - mean (959ms) : 939, 979
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8732) - mean (276ms) : 269, 282
master - mean (278ms) : 273, 282
section Bailout
This PR (8732) - mean (276ms) : 271, 281
master - mean (280ms) : 275, 286
section CallTarget+Inlining+NGEN
This PR (8732) - mean (1,156ms) : 1118, 1193
master - mean (1,159ms) : 1122, 1196
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8732) - mean (274ms) : 267, 282
master - mean (276ms) : 268, 284
section Bailout
This PR (8732) - mean (275ms) : 268, 282
master - mean (276ms) : 267, 286
section CallTarget+Inlining+NGEN
This PR (8732) - mean (1,040ms) : 992, 1088
master - mean (1,041ms) : 1002, 1080
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
bm1549
marked this pull request as ready for review
June 5, 2026 16:10
andrewlock
reviewed
Jun 8, 2026
| foreach (var kvp in parsedData.Configurations) | ||
| { | ||
| var entry = kvp.Value; | ||
| if (entry.Scope is null || !entry.Scope.Any(s => string.Equals(s, "managed", StringComparison.OrdinalIgnoreCase))) |
Member
There was a problem hiding this comment.
Avoid LINQ if possible as it's relatively expensive. Plus we should prob just do case-sensitive comparison
bm1549
added a commit
that referenced
this pull request
Jun 9, 2026
## 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]>
Parses scope into string[]? on ConfigurationEntry; the generator skips C# constant generation for native-only entries and emits DDSG0009 for missing or unrecognized scope values. Adds scope behavior tests. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Split the single loop into a validation pass (scope + documentation, for all entries) and a generation pass that filters to managed-scoped keys up front. Since scope is already validated, the generation pass can filter on "managed" directly. Behavior is unchanged; the flow now reads as "validate everything, then generate constants for managed keys." Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Co-authored-by: Andrew Lock <[email protected]>
…urationKeysGenerator.cs Co-authored-by: Andrew Lock <[email protected]>
…e checks
Address source-generator review feedback:
- Switch ConfigurationEntry.Scope/Aliases to EquatableArray<string>,
removing the hand-rolled ArraysEqual helper (anna-git, andrewlock).
- Replace the LINQ + OrdinalIgnoreCase managed-scope filter with an
allocation-free, case-sensitive ScopeContains helper (andrewlock), and
drop the now-unused ValidScopeValues field.
- Fix a pattern-precedence bug in scope validation: `is not "managed"
or "native"` parsed as `(not "managed") or "native"`, flagging every
non-"managed" value (including "native"); use `is not ("managed" or
"native")`.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
bm1549
force-pushed
the
brian.marks/native-config-scope-pr2
branch
from
June 9, 2026 22:47
a5b94f6 to
d823642
Compare
anna-git
reviewed
Jun 10, 2026
anna-git
approved these changes
Jun 10, 2026
anna-git
left a comment
anna-git
left a comment
Contributor
There was a problem hiding this comment.
NIT on one funny comment left and Andrew's comment to address on the usage of linq
…urationKeysGenerator.cs Co-authored-by: Anna <[email protected]>
The scope CSV parse was the only LINQ this PR introduced; swap the Split().Select().Where().ToArray() chain for an allocation-free loop, per review preference to avoid LINQ in the source generator. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
bm1549
enabled auto-merge (squash)
June 10, 2026 17:28
zacharycmontoya
pushed a commit
that referenced
this pull request
Jun 16, 2026
## 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]>
zacharycmontoya
pushed a commit
that referenced
this pull request
Jun 16, 2026
…3) (#8732) ## Summary of changes Adds the scope field infrastructure to the configuration registry: parsing, source-generator enforcement, tests, and documentation. Stack: - **PR 1** (merged): `scope: managed` on all existing entries - **PR 2 (this)**: scope infrastructure — parsing, enforcement, tests, docs - **PR 3**: all native config variables ## Reason for change Lets the registry distinguish managed-only, native-only, and dual-scope (`managed, native`) config variables. The generator emits C# constants only for entries whose scope includes `managed`; native-only entries are registered for coverage tracking and produce no constant. A build error (DDSG0009) is emitted for any entry missing a valid scope. ## Implementation details - `YamlReader.cs`: `ConfigurationEntry.Scope`/`Aliases` are now `EquatableArray<string>` for value-based equality (the incremental generator caches on it); comma-separated scope parsing (`managed`, `native`, `managed, native`). - `ConfigurationKeysGenerator.cs`: a validation pass emits DDSG0009 for missing/empty/unrecognized scope (`scopeValue is not ("managed" or "native")`); a separate generation pass emits constants only for managed-scoped keys, via an allocation-free, case-sensitive `ScopeContains` helper. - `ConfigKeyAliasesSwitcherGenerator.cs`: updated for the `EquatableArray<string>` aliases. - `ConfigurationKeysGeneratorTests.cs`: tests for native-only exclusion, `managed, native` inclusion, and DDSG0009 on empty/invalid scope; existing YAML fixtures carry `scope`. - `AddingConfigurationKeys.md`: `scope` documented as a Required field. ## Test coverage Source-generator unit tests pass, including the native-only exclusion and DDSG0009 cases. The native-coverage enforcement (a Nuke build step) lands in PR 3 alongside the data it validates. ## Other details Safe to merge onto master: all existing entries carry `scope: managed`, so DDSG0009 does not fire. --------- Co-authored-by: Claude Sonnet 4.6 <[email protected]> Co-authored-by: Andrew Lock <[email protected]> Co-authored-by: Anna <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Adds the scope field infrastructure to the configuration registry: parsing, source-generator enforcement, tests, and documentation.
Stack:
scope: managedon all existing entriesReason for change
Lets the registry distinguish managed-only, native-only, and dual-scope (
managed, native) config variables. The generator emits C# constants only for entries whose scope includesmanaged; native-only entries are registered for coverage tracking and produce no constant. A build error (DDSG0009) is emitted for any entry missing a valid scope.Implementation details
YamlReader.cs:ConfigurationEntry.Scope/Aliasesare nowEquatableArray<string>for value-based equality (the incremental generator caches on it); comma-separated scope parsing (managed,native,managed, native).ConfigurationKeysGenerator.cs: a validation pass emits DDSG0009 for missing/empty/unrecognized scope (scopeValue is not ("managed" or "native")); a separate generation pass emits constants only for managed-scoped keys, via an allocation-free, case-sensitiveScopeContainshelper.ConfigKeyAliasesSwitcherGenerator.cs: updated for theEquatableArray<string>aliases.ConfigurationKeysGeneratorTests.cs: tests for native-only exclusion,managed, nativeinclusion, and DDSG0009 on empty/invalid scope; existing YAML fixtures carryscope.AddingConfigurationKeys.md:scopedocumented as a Required field.Test coverage
Source-generator unit tests pass, including the native-only exclusion and DDSG0009 cases. The native-coverage enforcement (a Nuke build step) lands in PR 3 alongside the data it validates.
Other details
Safe to merge onto master: all existing entries carry
scope: managed, so DDSG0009 does not fire.