fix(otel)!: SignalOwnershipRegistry — end double instrumentation (CODE RED #3) - #23
Merged
Merged
Conversation
…tion (CODE RED #3) Confirmed HIGH double-count: when a consumer opted into more than one lane (source-generated interceptor, generated middleware, DiagnosticListener) for the same signal, the operation was instrumented twice — and the WebApiAotDemo "verified" fixture masked it (it captured both an aspnetcore.server and an http.server span for one endpoint and still passed). New QylSignalOwnership registry: each lane registers a priority for a signal (Interceptor 95 > GeneratedMiddleware 90 > DiagnosticListener 70); only the highest-priority registered lane emits, the rest defer — so exactly one span per operation. Race-free by construction: the interceptor path registers at endpoint-mapping time (Observe) and the interceptor helpers' static initializers register on first use — inside the intercepted call, before it reaches the framework code that raises the DiagnosticListener event — and the middleware registers at DI time. Wired: - DiagnosticListenerSubscriber: registers @70, defers in OnNext when a higher lane owns the signal. - AddQylAspNetCoreInstrumentation: registers @90; QylAspNetCoreStartupFilter middleware defers to the endpoint interceptor lane (95) so middleware + intercepted endpoints still yield one server span. - QylInterceptedAspNetCore.Observe: registers the ASP.NET Core interceptor lane @95. - QylInterceptedHttpClient / QylInterceptedGrpcNetClient: static ctors register HTTPCLIENT / GRPCNETCLIENT @95. Honest fixture: the WebApiAotDemo now emits exactly ONE server span per request (aspnetcore.server, route-backfilled); the http.server listener span is gone. The self-check + golden report were updated to assert the single-lane output, so any future re-double fails the gate. Verified: core.slnf 0/0; verify-webapi-aot-demo, source-interceptor-consumer, public-api-baseline (no API change — QylSignalOwnership is internal), generator-snapshots all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Introduces a per-instrumentation “signal ownership” mechanism intended to prevent double instrumentation when multiple lanes (interceptor / generated middleware / DiagnosticListener) are active for the same integration, and updates the WebApiAotDemo verified fixture to assert a single server-span lane.
Changes:
- Added
QylSignalOwnershipregistry with lane priorities and integrated it into middleware and DiagnosticListener emission decisions. - Registered interceptor ownership on first use for HttpClient and gRPC helpers, and at ASP.NET Core endpoint-mapping time.
- Updated WebApiAotDemo verification/reporting to expect
aspnetcore.server(single server span) and removed the priorhttp.serverexpectation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Qyl.OpenTelemetry.AutoInstrumentation.WebApiAotDemo/verified/report.json | Updates verified activity set to reflect single-lane server span (aspnetcore.server). |
| tools/Qyl.OpenTelemetry.AutoInstrumentation.WebApiAotDemo/Program.cs | Updates assertions and commentary for the server-span domain/lane behavior. |
| src/Qyl.OpenTelemetry.AutoInstrumentation/QylSignalOwnership.cs | Adds the new ownership/priority registry. |
| src/Qyl.OpenTelemetry.AutoInstrumentation/QylInterceptedHttpClient.cs | Registers interceptor-lane ownership on first helper use. |
| src/Qyl.OpenTelemetry.AutoInstrumentation/QylInterceptedGrpcNetClient.cs | Registers interceptor-lane ownership on first helper use. |
| src/Qyl.OpenTelemetry.AutoInstrumentation/QylInterceptedAspNetCore.cs | Claims interceptor ownership during endpoint mapping (Observe). |
| src/Qyl.OpenTelemetry.AutoInstrumentation/QylAspNetCoreStartupFilter.cs | Gates middleware emission based on ownership registry. |
| src/Qyl.OpenTelemetry.AutoInstrumentation/QylAspNetCoreInstrumentationServiceCollectionExtensions.cs | Registers middleware-lane ownership during DI setup. |
| src/Qyl.OpenTelemetry.AutoInstrumentation.DiagnosticListeners/DiagnosticListenerSubscriber.cs | Registers/gates DiagnosticListener emission via ownership registry. |
Comment on lines
+42
to
46
| // Claim the DiagnosticListener lane for this signal. If a higher-priority lane (interceptor / | ||
| // generated middleware) also covers it, this subscriber defers in OnNext so the operation is | ||
| // instrumented exactly once. See QylSignalOwnership. | ||
| QylSignalOwnership.Register(InstrumentationId, QylSignalOwnership.DiagnosticListener); | ||
| _allListenersSubscription ??= DiagnosticListener.AllListeners.Subscribe(new AllListenersObserver(this)); |
Comment on lines
+29
to
+31
| // Claim the ASP.NET Core signal for the middleware lane so the DiagnosticListener lane (if the | ||
| // Hosting package is also referenced) defers and the server span is emitted exactly once. | ||
| QylSignalOwnership.Register(QylAutoInstrumentationIds.AspNetCore, QylSignalOwnership.GeneratedMiddleware); |
Comment on lines
+151
to
+153
| // Server span is owned by the generated-middleware lane (priority 90) which wins over the | ||
| // DiagnosticListener lane (70) via QylSignalOwnership, so exactly one server span is emitted and | ||
| // it carries the aspnetcore.server domain (with the route backfilled after routing). |
Comment on lines
+14
to
+17
| // Registered on first use — which is inside an intercepted HttpClient call, before that call reaches | ||
| // the BCL that raises the HttpClient DiagnosticListener event — so the listener lane defers (no double). | ||
| static QylInterceptedHttpClient() | ||
| => QylSignalOwnership.Register(QylAutoInstrumentationIds.HttpClient, QylSignalOwnership.Interceptor); |
Comment on lines
+12
to
+15
| // Registered on first use (inside an intercepted gRPC call, before the underlying call raises the | ||
| // Grpc.Net.Client DiagnosticListener event) so the listener lane defers — no double-count. | ||
| static QylInterceptedGrpcNetClient() | ||
| => QylSignalOwnership.Register(QylAutoInstrumentationIds.GrpcNetClient, QylSignalOwnership.Interceptor); |
Comment on lines
+126
to
+131
| // The endpoint interceptor lane owns the ASP.NET Core server signal (priority 95). Registered here | ||
| // at endpoint-mapping time (before requests) so the middleware (90) and DiagnosticListener (70) | ||
| // lanes defer — exactly one server span per request. Registration is NOT done from InvokeAsync, | ||
| // which the middleware also calls; only actually-intercepted endpoints claim the interceptor lane. | ||
| QylSignalOwnership.Register(QylAutoInstrumentationIds.AspNetCore, QylSignalOwnership.Interceptor); | ||
| return requestDelegate is null ? null! : context => InvokeAsync(requestDelegate, context); |
ANcpLua
added a commit
that referenced
this pull request
Jul 1, 2026
…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>
ANcpLua
added a commit
that referenced
this pull request
Jul 1, 2026
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>
ANcpLua
added a commit
that referenced
this pull request
Jul 1, 2026
…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>
ANcpLua
added a commit
that referenced
this pull request
Jul 2, 2026
…elf-referential validation apparatus (#28) * fix(generator): throw on unknown DB instrumentation id in GetDbTraceContractKey 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> * feat(tcg): surface interceptor receiver types in the Telemetry Capability 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> * chore: task-state anchor for descriptor-metadata root fix Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(generator): body-descriptor hierarchy replaces policy metadata (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> * refactor(generator): make descriptor invariants structural, delete the 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> * test(invariants): verify the structural descriptor model, drop redundant-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> * chore: tick task-state checklist (implementation verified) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(fixtures): update webapi-aot OTLP trace fixture for main's semconv 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> * fix(tcg): publish the NServiceBus receiver surface the matcher actually 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> * refactor(generator)!: delete the unreachable AspNetCoreRequestDelegate 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> * chore: drop the task-state file — both tracked tasks verified complete 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> * docs: CLAUDE.md becomes the single agent-rules file; truthful server-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> * docs(tcg): point the generated-files rule reference at CLAUDE.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Resolves CODE RED #3 (double-count). When a consumer opted into >1 lane (interceptor / generated middleware / DiagnosticListener) for the same signal, the operation was instrumented twice — and the AOT "verified" fixture masked it.
QylSignalOwnership(your design): each lane registers a priority per signal — Interceptor 95 > GeneratedMiddleware 90 > DiagnosticListener 70 — only the highest emits, the rest defer → exactly one span. Race-free: interceptor path registers at endpoint-mapping (Observe) / first use (helper static ctors, inside the intercepted call before the listener event); middleware at DI time.AddQylAspNetCoreInstrumentation) registers @90 + defers to the endpoint interceptor lane.QylInterceptedAspNetCore.Observeclaims @95;QylInterceptedHttpClient/GrpcNetClientclaim @95.Honest fixture:
WebApiAotDemonow emits one server span per request (aspnetcore.server, route-backfilled);http.serveris gone; the self-check + golden report assert single-lane, so a future re-double fails the gate.Verified: core 0/0;
verify-webapi-aot-demo,source-interceptor-consumer,public-api-baseline(no API change — registry is internal),generator-snapshotsall pass.🤖 Generated with Claude Code