refactor(generator): make descriptor invariants structural — delete the self-referential validation apparatus - #28
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (6)CLAUDE.md📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**⚙️ CodeRabbit configuration file
Files:
⚙️ CodeRabbit configuration file
Files:
docs/TELEMETRY_CAPABILITY_GRAPH.md📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{cs,csproj}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/Qyl.OpenTelemetry.AutoInstrumentation/**/*.{cs,csproj}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.cs⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR replaces interceptor descriptor policy flags with typed body descriptors, rewires generator dispatch and detection around that model, updates invariant checks and manifest output, and refreshes repository guidance plus related docs and fixtures. ChangesGenerator and verification refactor
Repository guidance and evidence
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
Pull request overview
Refactors the source generator’s interceptor descriptor model to make previously runtime-validated invariants structural (via a closed body-descriptor hierarchy), removing a large self-referential validation layer and simplifying matcher/emission descriptor metadata. This aligns with the repo’s goal of keeping generator output deterministic while reducing consumer-build fragility.
Changes:
- Replaces “optional IsDefined structs + policy enums + validator apparatus” with a closed
InterceptorBodyDescriptorrecord hierarchy and type-switch emitter dispatch. - Slims matcher descriptors down to
(Name, ReceiverTypePattern, TryMatch)and removes kind-mask / contract-key / family / method-shape metadata that existed only for validation. - Updates
tools/verify-contract-invariants.pyto enforce the remaining real invariants (unique kind→body mapping, DB key derivation) without pinning deleted validator tokens.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tools/verify-contract-invariants.py | Updates invariant checks to match the new structural descriptor model and DB trace key derivation. |
| src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/TelemetryCapabilityGraphGenerator.cs | Adds interceptorReceivers emission to the TCG manifest JSON. |
| src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cs | Rewrites generated matcher/emission descriptor construction and exposes receiver surface for TCG. |
| src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Shapes.cs | Removes DB trace-key switch helper and kind-mask machinery; updates call sites accordingly. |
| src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cs | Derives DB trace contract keys directly from instrumentationId. |
| src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Descriptors.cs | Introduces InterceptorBodyDescriptor hierarchy; removes policy enums and matcher metadata fields. |
| src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.cs | Deletes descriptor catalog validation and matcher/emission cross-checks; switches emitter dispatch to body-type pattern matching. |
| .claude/TASK.md | Updates task documentation to reflect the new refactor plan and evidence. |
…ontractKey The default arm silently mapped any unlisted instrumentationId to signals.traces.ADONET — a false trace-contract identity, inconsistent with the sibling GetDbMetricContractKeys (empty default) and the codebase-wide 'unknown -> throw' invariant. All 7 ids GetDbInstrumentationId can return are enumerated explicitly, so the default is unreachable today; this makes a future unlisted provider fail loudly instead of misfiling. Verified: full solution build green (0/0); AspNetCore + ILogger runtime interception verifiers still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lity Graph InterceptorMatcherDescriptor.ReceiverTypePattern was a curated 25-row registry (incl. wildcard/pipe patterns like 'Azure.*Client' and the Kafka/MassTransit/Elastic unions) that nothing read — dead metadata. Rather than delete a curated column that is not recoverable from the matcher delegates, make it live: GetInterceptorReceiverSurface() reads it and the Telemetry Capability Graph now emits an 'interceptorReceivers' section, giving consumers a machine-readable map of the exact receiver surface qyl intercepts. Verified: solution build green (0/0); generator-snapshots-ok; tcg-publishing-demo-ok. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ta (cut 1/4) Delete the five validation-only enums (EmitterFamily, MethodShape, SignalOwnership, ErrorPolicy, DurationPolicy) — every read of them was inside validators that checked static data against redundant copies of itself. The 8 body descriptors become a closed sealed-record hierarchy under InterceptorBodyDescriptor; exactly-one-body is now structural. Matcher descriptor: 8 ctors -> 2, declaration metadata dropped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e validation apparatus (cuts 2-4) Every read of TargetKindMask, matcher ContractKeys, matcher/emission Family and MethodShape, the three policy enums, and the target's three Matcher* fields was inside validators comparing static data against a redundant copy of itself. With the body hierarchy from cut 1: - emission catalog rows shrink to (Kind, Body); exactly-one-body and policy consistency are unrepresentable as errors, not runtime-checked - matcher rows shrink to (Name, ReceiverTypePattern, TryMatch) - emitter dispatch is a type switch on the body descriptor - ValidateDescriptorCatalog, Ensure* trio, ValidateEmissionDescriptorPolicy, ValidateSingleBodyDescriptor, ValidateMethodShape x2, ValidatePolicy and the Initialize()-time throw (the CS8785/TreatWarningsAsErrors build-break vector) are deleted, not relocated - GetDbTraceContractKey was 'signals.traces.' + id re-enumerated as a switch; the id set now lives only in GetDbInstrumentationId and the contract key is derived, so the unreachable default arm (silent-ADONET before 544ee8d, throw after) is gone entirely - InterceptorKinds()/GetInterceptorKindMask bitmask machinery deleted Verified: solution build 0/0 (TWAE), generator-snapshots-ok (byte-identical emitted source), real-aspnetcore-demo-ok, real-ilogger-demo-ok. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ant-representation checks The python harness pinned the deleted validators as required tokens and re-derived SignalOwnership from target contract keys only to compare it against the declared enum — consistency checks between two encodings of one fact. Now that one encoding remains: - new parse_emission_descriptor_bodies: every InterceptorKind maps to exactly one typed body row, duplicates fail (the one real invariant ValidateDescriptorCatalog carried, now test-time instead of consumer build-time) - emitter dispatch check follows the body-type switch - DB trace contract keys derive from GetDbInstrumentationId (single source) instead of the deleted GetDbTraceContractKey switch - ownership/policy-matrix checks deleted with the enums they mirrored Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/TASK.md:
- Line 4: The TASK.md header has an inconsistent Started date because the
current value is in the future. Update the Started field in the task header to
the actual start date so it matches the review date and keep the same metadata
format.
- Around line 53-60: Add the missing blank lines around the markdown headings in
the TASK document so the lint passes: ensure `## Non-goals` and `## Resume
notes` each have a blank line before and after them, while leaving the existing
content and the non-goals/resume notes text unchanged.
In
`@src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cs`:
- Line 21: The NServiceBus interceptor manifest is advertising the wrong
receiver surface because QylGeneratedSourceInterceptorCatalog’s NServiceBus
matcher string and serialization path are out of sync with
TryGetNServiceBusInvocation. Update the descriptor and the TCG serialization so
they reflect the actual supported types for this binary, using the same
NServiceBus surface that TryGetNServiceBusInvocation accepts and emits. Make
sure the catalog entry and the generated manifest stay aligned so the published
capability graph documents current-tree behavior rather than a stale
IEndpointInstance/IPipelineContext surface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7757c443-a71a-4eb1-8106-f8b8b34e9bf3
📒 Files selected for processing (8)
.claude/TASK.mdsrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Descriptors.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Shapes.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/TelemetryCapabilityGraphGenerator.cstools/verify-contract-invariants.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: verify (qyl-macos)
🧰 Additional context used
📓 Path-based instructions (2)
**
⚙️ CodeRabbit configuration file
AGENTS.md
**: # Qyl.OpenTelemetry.AutoInstrumentation agent rulesMission
This repository is the runtime AOT auto-instrumentation lane for qyl, evolving into a
self-describing observability substrate. The foundation is unchanged: .NET 10
NativeAOT-compatible zero-code instrumentation through managed build assets, source generation,
DiagnosticListener consumption, and module-initializer boot. The direction is the North Star
below.Keep this repository separate from:
- semantic-convention package generation (
Qyl.OpenTelemetry.SemanticConventionsis a referenced
vocabulary package, not generated here),- the old CLR-profiler/OpenTelemetry auto-instrumentation substrate.
North Star — declare and prove the whole stack
Every observability tool today is pull-by-observation: a backend learns what a service emits
by receiving samples over time, and never knows whether it has seen the whole surface. qyl has a
capability none of them have — because instrumentation is source-generated interceptors + a static
contract + a referenced semconv registry + (incrementally) DTO inference, the complete set of
telemetry a binary can ever produce is a compile-time-derivable fact, with provenance.The substrate goal: every qyl binary ships a complete, machine-readable Telemetry Capability
Graph (TCG) — the full possible OpenTelemetry surface for that exact binary, each capability
tagged compile-time-owned vs runtime-valued — and proves it by self-hosting (instrumenting its
own pipeline with its own mechanism, zero extra code). Any external entity consumes the TCG to know
the entire stack before a span is sampled. The contract becomes the shared semantic graph; an OTLP
backend is just one consumer.Three pillars:
- Self-host (the proof). qyl instruments qyl with qyl —
QylSelfTelemetry/
SemConvConformanceProcessorare the seed; the binary observing itself is how "declared TCG ==
runtime reality" is checked.- **Compile-time-complete ...
Files:
src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/TelemetryCapabilityGraphGenerator.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Shapes.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Descriptors.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.cstools/verify-contract-invariants.py
⚙️ CodeRabbit configuration file
**: Operating principles (solo-dev, agentic SDLC — reviews are advisory, agents act on them):
- LAZY: one self-contained, correct review beats ten partial ones. Every finding is
definitive — concrete evidence with file:line, a concrete fix, no "consider maybe",
no open or ambiguous questions back to the author. If you cannot decide a point
from the diff plus repo context, stay silent on it. Never cite a source, API, or
version you have not verified; an unverifiable claim is a dropped claim.- IMPATIENT: never stall a PR. There are no compatibility obligations here — internal
and dogfooding code has NO public-API contract; removing shims, breaking signatures,
and deleting dead paths are normal, desirable changes. Do not flag backward
compatibility, deprecation ceremony, or migration paths. (SemVer applies only to
commercially sold libraries — this repo has none.)- EGO: hold the bar of the best reviewer on the market — flag real correctness,
security, data-loss, and structural problems precisely; produce zero noise.
Files:
src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/TelemetryCapabilityGraphGenerator.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Shapes.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Descriptors.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.cstools/verify-contract-invariants.py
src/**/*.cs
⚙️ CodeRabbit configuration file
src/**/*.cs: Zero-code instrumentation runtime: this code runs inside EVERY request of host
applications. Top priorities, in order: (1) allocations and boxing on hot paths —
flag closures, LINQ, params arrays, string concat in listener/semantic-tag code;
(2) tag cardinality — any attribute value that is unbounded (raw URLs, user input,
exception messages) explodes at scale; (3) Activity/Meter lifecycle — undisposed
listeners, leaked subscriptions, double-Start/Stop; (4) thread safety of shared
listener state. PublicAPI.Shipped/Unshipped.txt are analyzer-managed: edits must
come from the analyzer flow, and API breaks are fine (internal product, no
compatibility contract).
Files:
src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/TelemetryCapabilityGraphGenerator.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Shapes.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Descriptors.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.cs
🪛 markdownlint-cli2 (0.22.1)
.claude/TASK.md
[warning] 53-53: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 58-58: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (7)
tools/verify-contract-invariants.py (1)
1352-1396: LGTM!Also applies to: 1446-1458, 1467-1467, 1491-1504, 1538-1538, 1554-1554, 1587-1587
src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Descriptors.cs (1)
10-41: LGTM!Also applies to: 93-150, 263-344
src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.cs (1)
65-85: LGTM!Also applies to: 142-222, 253-262, 309-341, 360-386, 404-435, 551-555
src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cs (1)
146-146: LGTM!src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Shapes.cs (1)
96-107: LGTM!Also applies to: 855-856
src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cs (1)
9-20: LGTM!Also applies to: 22-97
src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/TelemetryCapabilityGraphGenerator.cs (1)
119-132: LGTM!
…v changes PR #23/#25 changed the aspnetcore server-span shape on main (qyl.instrumentation.domain http.server -> aspnetcore.server; new url.scheme attribute) without regenerating this fixture, and main's own verify runs are still queued — so any PR merge-commit run fails 'otlp verified fixtures'. Regenerated via verify-otlp-fixtures.py --update-verified; the diff is exactly those two attribute changes, unrelated to the descriptor refactor (generator snapshots byte-identical). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2f68501 to
3d74049
Compare
…ly accepts IsNServiceBusEndpointType matches IMessageSession | IMessageHandlerContext; the curated pattern still claimed IMessageSession | IEndpointInstance | IPipelineContext from an earlier design. Now that GetInterceptorReceiverSurface serializes the pattern into the Telemetry Capability Graph, the string must tell the truth. All other 24 patterns spot-checked against their matchers - only NServiceBus drifted. Also MD022 blank lines in the task file. Reported-by: coderabbitai Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cs`:
- Around line 11-30: Update QylGeneratedSourceInterceptorCatalog so the exported
receiver-surface metadata matches what the interceptor logic can actually emit.
The entries for AspNetCoreEndpointMap, LoggerExtensions, and MongoDb should
advertise the real receiver types used by the corresponding
TryGetAspNetCoreEndpointMapInvocation, TryGetLoggerExtensionInvocation, and
TryGetMongoDbInvocation logic, not the matcher labels; also remove the dead
AspNetCoreRequestDelegate surface entry since the RequestDelegate.Invoke
MethodKind guard makes it non-matchable. Keep the receiver surface explicit and
aligned with each InterceptorTarget.ReceiverType so
GetInterceptorReceiverSurface reflects current-tree behavior only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7d7464b5-c3ec-41ea-b948-22a180264a87
📒 Files selected for processing (9)
.claude/TASK.mdsrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Descriptors.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Shapes.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/TelemetryCapabilityGraphGenerator.cstools/Qyl.OpenTelemetry.AutoInstrumentation.OtlpFixtures/verified/webapi-aot-traces.otlp.jsontools/verify-contract-invariants.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: verify (qyl-macos)
🧰 Additional context used
📓 Path-based instructions (2)
**
⚙️ CodeRabbit configuration file
AGENTS.md
**: # Qyl.OpenTelemetry.AutoInstrumentation agent rulesMission
This repository is the runtime AOT auto-instrumentation lane for qyl, evolving into a
self-describing observability substrate. The foundation is unchanged: .NET 10
NativeAOT-compatible zero-code instrumentation through managed build assets, source generation,
DiagnosticListener consumption, and module-initializer boot. The direction is the North Star
below.Keep this repository separate from:
- semantic-convention package generation (
Qyl.OpenTelemetry.SemanticConventionsis a referenced
vocabulary package, not generated here),- the old CLR-profiler/OpenTelemetry auto-instrumentation substrate.
North Star — declare and prove the whole stack
Every observability tool today is pull-by-observation: a backend learns what a service emits
by receiving samples over time, and never knows whether it has seen the whole surface. qyl has a
capability none of them have — because instrumentation is source-generated interceptors + a static
contract + a referenced semconv registry + (incrementally) DTO inference, the complete set of
telemetry a binary can ever produce is a compile-time-derivable fact, with provenance.The substrate goal: every qyl binary ships a complete, machine-readable Telemetry Capability
Graph (TCG) — the full possible OpenTelemetry surface for that exact binary, each capability
tagged compile-time-owned vs runtime-valued — and proves it by self-hosting (instrumenting its
own pipeline with its own mechanism, zero extra code). Any external entity consumes the TCG to know
the entire stack before a span is sampled. The contract becomes the shared semantic graph; an OTLP
backend is just one consumer.Three pillars:
- Self-host (the proof). qyl instruments qyl with qyl —
QylSelfTelemetry/
SemConvConformanceProcessorare the seed; the binary observing itself is how "declared TCG ==
runtime reality" is checked.- **Compile-time-complete ...
Files:
src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/TelemetryCapabilityGraphGenerator.cstools/Qyl.OpenTelemetry.AutoInstrumentation.OtlpFixtures/verified/webapi-aot-traces.otlp.jsonsrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Shapes.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.cstools/verify-contract-invariants.pysrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Descriptors.cs
⚙️ CodeRabbit configuration file
**: Operating principles (solo-dev, agentic SDLC — reviews are advisory, agents act on them):
- LAZY: one self-contained, correct review beats ten partial ones. Every finding is
definitive — concrete evidence with file:line, a concrete fix, no "consider maybe",
no open or ambiguous questions back to the author. If you cannot decide a point
from the diff plus repo context, stay silent on it. Never cite a source, API, or
version you have not verified; an unverifiable claim is a dropped claim.- IMPATIENT: never stall a PR. There are no compatibility obligations here — internal
and dogfooding code has NO public-API contract; removing shims, breaking signatures,
and deleting dead paths are normal, desirable changes. Do not flag backward
compatibility, deprecation ceremony, or migration paths. (SemVer applies only to
commercially sold libraries — this repo has none.)- EGO: hold the bar of the best reviewer on the market — flag real correctness,
security, data-loss, and structural problems precisely; produce zero noise.
Files:
src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/TelemetryCapabilityGraphGenerator.cstools/Qyl.OpenTelemetry.AutoInstrumentation.OtlpFixtures/verified/webapi-aot-traces.otlp.jsonsrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Shapes.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.cstools/verify-contract-invariants.pysrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Descriptors.cs
src/**/*.cs
⚙️ CodeRabbit configuration file
src/**/*.cs: Zero-code instrumentation runtime: this code runs inside EVERY request of host
applications. Top priorities, in order: (1) allocations and boxing on hot paths —
flag closures, LINQ, params arrays, string concat in listener/semantic-tag code;
(2) tag cardinality — any attribute value that is unbounded (raw URLs, user input,
exception messages) explodes at scale; (3) Activity/Meter lifecycle — undisposed
listeners, leaked subscriptions, double-Start/Stop; (4) thread safety of shared
listener state. PublicAPI.Shipped/Unshipped.txt are analyzer-managed: edits must
come from the analyzer flow, and API breaks are fine (internal product, no
compatibility contract).
Files:
src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/TelemetryCapabilityGraphGenerator.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Shapes.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.cssrc/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Descriptors.cs
🔇 Additional comments (3)
.claude/TASK.md (1)
4-4: Fix the Started date.
2026-07-02is still in the future for this review date (2026-07-01). Update the header to the actual start date.tools/verify-contract-invariants.py (1)
1352-1396: LGTM!Also applies to: 1446-1458, 1467-1467, 1491-1504, 1538-1538, 1554-1554, 1587-1587
tools/Qyl.OpenTelemetry.AutoInstrumentation.OtlpFixtures/verified/webapi-aot-traces.otlp.json (1)
24-24: LGTM!Also applies to: 56-62
| new InterceptorMatcherDescriptor("AspNetCoreRequestDelegate", "global::Microsoft.AspNetCore.Http.RequestDelegate", TryGetAspNetCoreRequestDelegateInvocation), | ||
| new InterceptorMatcherDescriptor("AspNetCoreEndpointMap", "global::Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions", TryGetAspNetCoreEndpointMapInvocation), | ||
| new InterceptorMatcherDescriptor("MeterProviderBuilderAddMeter", "global::OpenTelemetry.Metrics.MeterProviderBuilder", TryGetMeterProviderBuilderAddMeterInvocation), | ||
| new InterceptorMatcherDescriptor("AzureClient", "Azure.*Client", TryGetAzureClientInvocation), | ||
| new InterceptorMatcherDescriptor("Elastic", "Elastic.Clients.Elasticsearch.*Client|Elastic.Transport.ITransport", TryGetElasticInvocation), | ||
| new InterceptorMatcherDescriptor("WcfClient", "global::System.ServiceModel.ClientBase<TChannel>", TryGetWcfClientInvocation), | ||
| new InterceptorMatcherDescriptor("GrpcNetClientUnary", "global::Grpc.Core.ClientBase<T>", TryGetGrpcNetClientAsyncUnaryInvocation), | ||
| new InterceptorMatcherDescriptor("GrpcNetClientStreaming", "global::Grpc.Core.ClientBase<T>", TryGetGrpcNetClientStreamingInvocation), | ||
| new InterceptorMatcherDescriptor("Kafka", "Confluent.Kafka.IProducer<TKey,TValue>|Confluent.Kafka.IConsumer<TKey,TValue>", TryGetKafkaInvocation), | ||
| new InterceptorMatcherDescriptor("MassTransit", "MassTransit.IPublishEndpoint|MassTransit.ISendEndpoint|MassTransit.ISendEndpointProvider", TryGetMassTransitInvocation), | ||
| new InterceptorMatcherDescriptor("NServiceBus", "NServiceBus.IMessageSession|NServiceBus.IMessageHandlerContext", TryGetNServiceBusInvocation), | ||
| new InterceptorMatcherDescriptor("Quartz", "Quartz.IJob", TryGetQuartzInvocation), | ||
| new InterceptorMatcherDescriptor("StackExchangeRedis", "StackExchange.Redis.IDatabase", TryGetStackExchangeRedisInvocation), | ||
| new InterceptorMatcherDescriptor("GraphQL", "GraphQL.IDocumentExecuter", TryGetGraphQlInvocation), | ||
| new InterceptorMatcherDescriptor("EntityFrameworkCoreDbContext", "global::Microsoft.EntityFrameworkCore.DbContext", TryGetEntityFrameworkCoreDbContextInvocation), | ||
| new InterceptorMatcherDescriptor("EntityFrameworkCoreQueryable", "global::Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions", TryGetEntityFrameworkCoreQueryableInvocation), | ||
| new InterceptorMatcherDescriptor("MongoDb", "MongoDB.Driver.IMongoCollection<TDocument>", TryGetMongoDbInvocation), | ||
| new InterceptorMatcherDescriptor("DbCommand", "global::System.Data.Common.DbCommand", TryGetDbCommandInvocation), | ||
| new InterceptorMatcherDescriptor("RabbitMq", "RabbitMQ.Client.IModel|RabbitMQ.Client.IChannel", TryGetRabbitMqInvocation), | ||
| new InterceptorMatcherDescriptor("LoggerExtensions", "global::Microsoft.Extensions.Logging.LoggerExtensions", TryGetLoggerExtensionInvocation), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not publish matcher labels as the receiver surface.
GetInterceptorReceiverSurface() serializes ReceiverTypePattern, but several entries are not the emitted receiver surface: Line 12 publishes EndpointRouteBuilderExtensions while detection emits IEndpointRouteBuilder, Line 30 publishes LoggerExtensions while detection emits ILogger, Line 23 publishes IDatabase while detection gates IDatabaseAsync, and Line 11 is intentionally non-matchable due the RequestDelegate.Invoke MethodKind guard. The TCG will advertise receivers this binary cannot actually produce. Make receiver-surface metadata explicit and aligned with each InterceptorTarget.ReceiverType, and drop the dead RequestDelegate receiver from the exported surface.
Targeted fix shape
- new InterceptorMatcherDescriptor("AspNetCoreRequestDelegate", "global::Microsoft.AspNetCore.Http.RequestDelegate", TryGetAspNetCoreRequestDelegateInvocation),
- new InterceptorMatcherDescriptor("AspNetCoreEndpointMap", "global::Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions", TryGetAspNetCoreEndpointMapInvocation),
+ new InterceptorMatcherDescriptor("AspNetCoreEndpointMap", "global::Microsoft.AspNetCore.Routing.IEndpointRouteBuilder", TryGetAspNetCoreEndpointMapInvocation),
...
- new InterceptorMatcherDescriptor("StackExchangeRedis", "StackExchange.Redis.IDatabase", TryGetStackExchangeRedisInvocation),
+ new InterceptorMatcherDescriptor("StackExchangeRedis", "StackExchange.Redis.IDatabaseAsync", TryGetStackExchangeRedisInvocation),
...
- new InterceptorMatcherDescriptor("LoggerExtensions", "global::Microsoft.Extensions.Logging.LoggerExtensions", TryGetLoggerExtensionInvocation),
+ new InterceptorMatcherDescriptor("LoggerExtensions", "global::Microsoft.Extensions.Logging.ILogger", TryGetLoggerExtensionInvocation),As per coding guidelines, the TCG is “the full possible OpenTelemetry surface for that exact binary” and documentation must “Document current-tree behavior.”
Also applies to: 90-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cs`
around lines 11 - 30, Update QylGeneratedSourceInterceptorCatalog so the
exported receiver-surface metadata matches what the interceptor logic can
actually emit. The entries for AspNetCoreEndpointMap, LoggerExtensions, and
MongoDb should advertise the real receiver types used by the corresponding
TryGetAspNetCoreEndpointMapInvocation, TryGetLoggerExtensionInvocation, and
TryGetMongoDbInvocation logic, not the matcher labels; also remove the dead
AspNetCoreRequestDelegate surface entry since the RequestDelegate.Invoke
MethodKind guard makes it non-matchable. Keep the receiver surface explicit and
aligned with each InterceptorTarget.ReceiverType so
GetInterceptorReceiverSurface reflects current-tree behavior only.
Source: Coding guidelines
There was a problem hiding this comment.
Verified each sub-claim against the matchers; split verdict, fixed in the latest push:
Accepted — RequestDelegate row is dead (root-fixed harder than suggested): TryGetAspNetCoreRequestDelegateInvocation requires MethodKind.Ordinary, but a delegate's Invoke is always MethodKind.DelegateInvoke — unmatchable by construction since 770a897. Rather than just dropping it from the export, the whole path is deleted (matcher row, emission row, detection method, enum member). Server spans still come from the IStartupFilter middleware; aspnetcore-middleware-delegate-ok + real-aspnetcore-demo-ok confirm.
Accepted — Redis: matcher gates IDatabaseAsync; the surface now says StackExchange.Redis.IDatabaseAsync.
Rejected — EndpointRouteBuilderExtensions / LoggerExtensions are not 'matcher labels': those matchers literally gate on the extension class as the containing type (Detection.cs: IsType(original.ContainingType, "...EndpointRouteBuilderExtensions")), same convention as the third extension-method matcher (EFCore EntityFrameworkQueryableExtensions). The column documents the matched API surface, which for extension methods is the defining extensions class; collapsing LoggerExtensions to ILogger would duplicate the existing ILogger row and lose the direct-call vs extension-call distinction.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
fd -e cs QylAutoInstrumentationGenerator.Detection.cs
echo "---LoggerExtension---"
rg -n -B3 -A15 'TryGetLoggerExtensionInvocation' src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cs
echo "---EndpointMap---"
rg -n -B3 -A20 'TryGetAspNetCoreEndpointMapInvocation' src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cs
echo "---EFCoreQueryable---"
rg -n -B3 -A15 'TryGetEntityFrameworkCoreQueryableInvocation' src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cs
echo "---RequestDelegate removal check---"
rg -n 'RequestDelegate' src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/*.cs
echo "---Redis surface check---"
rg -n 'StackExchangeRedis' src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/*.csLength of output: 5351
@ANcpLua Confirmed against source, no further pushback:
- RequestDelegate: verified only remaining reference is line 202 (internal disambiguation inside
TryGetAspNetCoreEndpointMapInvocation, not a standalone matcher/emission/enum path). Dead path is fully gone. - Redis:
QylGeneratedSourceInterceptorCatalog.csline 22 now readsStackExchange.Redis.IDatabaseAsync, matching the gate inTryGetStackExchangeRedisInvocation. - EndpointRouteBuilderExtensions / LoggerExtensions: withdrawn.
TryGetEntityFrameworkCoreQueryableInvocation(Detection.cs line 1201) gates onEntityFrameworkQueryableExtensionsthe same way, and that row was never in question — so the extension-class-as-matched-surface convention is pre-existing and applied uniformly across all three extension-method matchers, not something introduced here. CollapsingLoggerExtensionsintoILoggerwould in fact break that convention and merge two distinct matcher identities into one row. My original claim was wrong on this point.
✏️ Learnings added
Learnt from: ANcpLua
Repo: ANcpLua/Qyl.OpenTelemetry.AutoInstrumentation PR: 28
File: src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylGeneratedSourceInterceptorCatalog.cs:0-0
Timestamp: 2026-07-01T23:14:59.040Z
Learning: In src/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators/QylAutoInstrumentationGenerator.Detection.cs, extension-method matchers (e.g. TryGetAspNetCoreEndpointMapInvocation gating on EndpointRouteBuilderExtensions, TryGetLoggerExtensionInvocation gating on LoggerExtensions, TryGetEntityFrameworkCoreQueryableInvocation gating on EntityFrameworkQueryableExtensions) intentionally publish the extension class as the matcher's receiver-type/surface identity in QylGeneratedSourceInterceptorCatalog.cs, even though the resulting InterceptorTarget.ReceiverType at runtime may be the actual instance interface (e.g. IEndpointRouteBuilder). This is an established, uniformly-applied convention distinguishing extension-call matchers from direct-instance-call matchers (e.g. the separate ILogger row) — it is not a defect to be "fixed" by collapsing to the instance type.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…e interceptor path; truthful Redis receiver surface TryGetAspNetCoreRequestDelegateInvocation required MethodKind.Ordinary on RequestDelegate.Invoke — but a delegate's Invoke is always MethodKind.DelegateInvoke, so the matcher has been unmatchable by construction since 770a897 added the guard. Server-span coverage comes from the IStartupFilter middleware (d31d94f); the generator path was dead weight advertised as live surface in the TCG. Deleted: matcher row, emission row, detection method, enum member. The runtime QylInterceptedAspNetCore.InvokeAsync helper stays (startup filter uses it). Redis: the matcher gates StackExchange.Redis.IDatabaseAsync; publish that instead of the narrower IDatabase. Verified: build 0/0, generator-snapshots-ok (byte-identical), contract-invariants-ok, aspnetcore-middleware-delegate-ok, tcg-publishing-demo-ok, real-aspnetcore-demo-ok. Reported-by: coderabbitai (receiver-surface truthfulness) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Build()-interceptor/IStartupFilter rewire (PR #20) is merged, released (v4.0.x on the feed), and pinned by qyl at 4.0.0; its two follow-up gaps were closed by later commits (route backfill after next() in QylInterceptedAspNetCore.RecordResponse, single-owner-per-signal registry in PR #23). The descriptor-metadata root fix is this PR — documented by the PR itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…span combination docs - AGENTS.md deleted, CLAUDE.md is now the real file (was a symlink) — this repo's agent rules are consumed via CLAUDE.md only - rules updated to current tree: structural descriptor model + invariants routed to verify-contract-invariants.py; no-callsite-arbitration invariant (the deleted Build()-interceptor coordination protocol stays deleted); publish vs build version roles stated per version-sync; note on not racing local verifier runs against CI on the shared self-hosted hosts - AddQylAspNetCoreInstrumentation: remark predated the single-owner signal registry (PR #23) — combining with .Hosting is safe (listener lane defers), say so; document IStartupFilter registration-order contract (server span stays outermost) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… left unstructural High-effort review of 13bcb38 (which subsumed fix/dbcontract-default-throw-and-dead-field via squash) confirmed the deleted self-referential validators left six invariants with no structural or gate replacement. This makes each one structural or gate-enforced: - TraceInterceptorBodyDescriptor.RuntimeHelper is now a required constructor parameter (was optional-with-default but dereferenced unconditionally — an omitted helper emitted syntactically broken code into consumer compilations); drop the dead IsDefined column from TraceRuntimeHelperDescriptor. - Logger level mapping is single-sourced: detection and emission share GetLoggerExtensionLevelName, and an unknown method name now throws at generation instead of silently forwarding at LogLevel.None (records dropped invisibly). - Delete the never-constructed LoggerInterceptorBodyKind.None member. - Deduplicate GetDbSystemName into QylDbActivityPolicy (the QylDbClientMetrics copy was a truncated fork), enumerate ADONET explicitly, and throw on unknown ids per the codebase-wide unknown->throw invariant. - verify-contract-invariants: new verify_matcher_registration gate (an unregistered or orphaned TryGet*Invocation detection method previously passed every gate — mutation- verified silent telemetry loss); derive the emitter-dispatch case list structurally from declared BodyDescriptor types instead of a hand-pinned 8-token mirror (a 9th body type without a case now fails the gate); make emission-row parsing wrap-tolerant; cache read_generator_sources. - verify-aot-publish-gate: approve runtime-pack vendor warnings at 10.0.10 (SDK patch moved packs from 10.0.9; same diagnostics, fail-closed drift check fired as designed). Both new gate checks mutation-tested (deleted matcher row and phantom body type each go red). Full handoff gate green: aot-autoinstrumentation-goal-ok. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What
Audit finding: every read of
TargetKindMask, matcherContractKeys, matcher/emissionFamily/MethodShape, the three policy enums (SignalOwnership/ErrorPolicy/DurationPolicy), and the target's threeMatcher*fields sat inside validators that compared static data against a redundant copy of itself.ValidateEmissionDescriptorPolicyitself proved the policy enums are fully determined by the body type — denormalized derived data, stored 29×, then runtime-checked for consistency on every consumer build.This PR makes the invariants structural instead of runtime-checked:
IsDefined-structs →abstract record InterceptorBodyDescriptor+ 8 sealed records. An emission row is now(Kind, Body); exactly-one-body and policy consistency are unrepresentable as errors.(Name, ReceiverTypePattern, TryMatch)— 8 constructors → 2. The declaration metadata (kind masks, contract-key lists) existed only to be validated against what the matchers actually produce.ValidateDescriptorCatalog+ itsInitialize()call (the CS8785 ×TreatWarningsAsErrors=trueconsumer-build-break vector),EnsureTargetDeclaredByMatcher/EnsureKindDeclaredByMatcher/EnsureContractDeclaredByMatcher,EnsureEmissionDescriptorMatchesMatcher,ValidateEmissionDescriptorPolicy,ValidateSingleBodyDescriptor,ValidateMethodShape×2,ValidatePolicy,InterceptorKinds()/GetInterceptorKindMaskbitmask machinery, and 5 enums.GetDbTraceContractKeywas"signals.traces." + idre-enumerated as a switch (its default arm was unreachable — silent-ADONET before 544ee8d, throw after). The id set now lives only inGetDbInstrumentationId; the key is derived. Both default arms are gone.tools/verify-contract-invariants.pyrewritten where it pinned the deleted validators as required tokens or re-derived ownership only to compare against the declared enum. The one real invariantValidateDescriptorCatalogcarried — everyInterceptorKindmaps to exactly one emission row — is now enforced test-time byparse_emission_descriptor_bodies(duplicates fail), not on every consumer build.GetInterceptorReceiverSurface()/ the TCGinterceptorReceiverssection is deliberately kept (consumed byTelemetryCapabilityGraphGenerator).Why
The generator carried a self-referential validation layer: metadata that exists only to be validated, validated by code that exists only because the metadata is redundant. Under
TreatWarningsAsErrors=truetheInitialize()-time throw was also the repo's only realistic consumer-build-break vector (CS8785 → error). Root fix over symptom fix: make the invariants structural and the whole layer deletes itself.Verification — complete and verified
tools/verify-generator-snapshots.py: generator-snapshots-ok — emitted source is byte-identical; this refactor changes zero generated bytestools/verify-contract-invariants.py: contract-invariants-ok🤖 Generated with Claude Code