coderabbit: maxed advisory config — telemetry-focused path tuning - #3
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Cache: Disabled due to Reviews > Disable Cache setting Knowledge base: Disabled due to data retention organization setting 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds ChangesCodeRabbit Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 445f4529fb
ℹ️ 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".
| labeling: | ||
| auto_apply_labels: true | ||
|
|
||
| knowledge_base: |
There was a problem hiding this comment.
With standard YAML parsing, this second top-level knowledge_base mapping replaces the earlier one rather than merging with it; parsing this file leaves only web_search, code_guidelines, and automatic_repository_linking, so the previously configured learnings, issues, pull_requests, mcp, and integration settings above are silently dropped. This makes the intended review context features ineffective unless both blocks are combined into a single knowledge_base section.
Useful? React with 👍 / 👎.
…g parsing Same latent bug as ancplua-skills PR #4: two top-level `knowledge_base:` mappings (lines 263 + 302). YAML forbids duplicate mapping keys, so CodeRabbit could not parse the file and silently fell back to the stale Organization-UI config — the maxed advisory config was inert on every PR. ajv passed earlier because YAML→JSON collapses the duplicate (last-key-wins) before the schema ever sees it; only CodeRabbit's strict YAML parser rejects it. Merged the second block's unique settings into the canonical block (`COVERAGE_LEDGER.md` filePattern + `automatic_repository_linking: true`), then deleted the duplicate. Validated: parses with zero duplicate keys, passes schema.v2.json. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ANcpLua
added a commit
that referenced
this pull request
Jul 1, 2026
…trument (CODE RED #1/#2/#10) (#22) An adversarial OTel-compliance + honesty audit rated this code 3/10. This lands the verified subset of the confirmed HIGH/MEDIUM defects. #1 (HIGH, ~0-duration lie): the HttpClient/AspNetCore/gRPC DiagnosticListeners created their span on the framework's *.Stop event via the 2-arg StartActivity (StartTimeUtc=now) and immediately disposed it, so every emitted span had ~0 duration instead of the real operation latency. New internal QylActivitySource.StartAtAmbientStart(name, kind) stamps the span to the ambient framework Activity's real StartTimeUtc (parented to it for trace correlation), with a now-stamped fallback when there is no ambient activity. All three listeners switched to it. #2 (HIGH, dishonest doc): DiagnosticListenerSubscriber claimed it "publishes the same span shapes" — false while durations were fabricated. Doc now states the actual mechanism (reacts on *.Stop, stamps to ambient start for real duration). #10 (MEDIUM, metrics-semconv): dotnet.thread_pool.thread.count was an unitless ObservableGauge<int>; per OTel semconv it is an (Observable)UpDownCounter with UCUM unit {thread}. Switched instrument type + added unit. Verified: core.slnf Release 0/0; verify-webapi-aot-demo passes (fixture unchanged — the attribute shape is identical; note the fixture is duration-insensitive, which is itself tracked as a separate finding). No PublicAPI change (all internal/private). NOT in this commit (separate verified increments): #3 double-count between the listener and interceptor lanes (+ the fixture that masks it), #4/#11 OTLP Events/Links/Status.Message dropped on ingest, #6 url.scheme, #7 http.request.method_original on the interceptor path, #9 Azure span name ignores methodName. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ANcpLua
added a commit
that referenced
this pull request
Jul 1, 2026
…tion (CODE RED #3) (#23) 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>
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
1 participant
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.
Maxed advisory
.coderabbit.yamlfor the instrumentation runtime. CodeRabbit's own review on this PR is the acceptance test.The three principles (global path instruction): LAZY — one definitive, self-contained review, evidence with file:line or silence, no open questions; IMPATIENT — no backcompat ceremony, breaking is normal (internal product; PublicAPI.txt is analyzer flow, not a compatibility contract); EGO — best-reviewer-on-the-market bar, zero noise.
Repo tuning (priority order): hot-path allocations/boxing in listener code, unbounded tag cardinality (explodes at scale), Activity/Meter lifecycle leaks, listener thread safety; semconv names/stability verified against the actual spec; deterministic tests (no sleeps, assert exported telemetry) + COVERAGE_LEDGER discipline; benchmark validity; bounded workflows.
Mechanics: loop brake, per-PR
skip-reviewlabel +[skip review]title,knowledge_base(CLAUDE/AGENTS/COVERAGE_LEDGER guidelines, web search, automatic repository linking).Advisory, never blocking. Every key validated against
schema.v2.json(ajv, draft2020).🤖 Generated with Claude Code