fix(otel)!: real span duration + honest docs + thread-count instrument (CODE RED 1/3) - #22
Conversation
…trument (CODE RED #1/#2/#10) 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>
|
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)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (2)**⚙️ CodeRabbit configuration file
Files:
⚙️ CodeRabbit configuration file
Files:
src/**/*.cs⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an internal StartAtAmbientStart helper to QylActivitySource that starts activities using the ambient Activity.Current's context and start time when present, wires three diagnostic listeners (AspNetCore, gRPC client, HttpClient) to use it, updates subscriber documentation, and changes ThreadPoolThreads metric from ObservableGauge to ObservableUpDownCounter. ChangesAmbient-Start Activity Timing
Thread Pool Metric Type
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant DiagnosticListener
participant QylActivitySource
participant ActivityCurrent
DiagnosticListener->>QylActivitySource: StartAtAmbientStart(operationName, kind)
QylActivitySource->>ActivityCurrent: read Activity.Current
alt ambient activity present
QylActivitySource->>QylActivitySource: StartActivity(name, kind, Context, startTime: ambient.StartTimeUtc)
else no ambient activity
QylActivitySource->>QylActivitySource: StartActivity(name, kind)
end
QylActivitySource-->>DiagnosticListener: Activity or null
Estimated code review effort: 2 (Simple) | ~12 minutes QylActivitySource.cs:36 — when no ambient Activity exists, StartAtAmbientStart falls through to StartActivity(operationName, activityKind) without Context, silently dropping any parent linkage that Source.StartActivity's default overload would have picked up from Activity.Current anyway — redundant branch, not a bug, but the null-check-then-reimplement-default pattern here is pointless code duplication. Ship it. 🚥 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
This PR addresses OTel semantic compliance issues in the DiagnosticListener-based tracing bridges by ensuring emitted qyl spans reflect real operation durations (rather than ~0-duration spans created on *.Stop), updates related documentation to accurately describe the mechanism, and corrects the runtime thread-pool thread-count metric instrument type/unit to match semconv.
Changes:
- Introduce
QylActivitySource.StartAtAmbientStart(...)to start qyl Activities at the ambient frameworkActivity’s start timestamp. - Switch HttpClient / ASP.NET Core / gRPC DiagnosticListeners to use
StartAtAmbientStartwhen creating spans on*.Stop. - Update
dotnet.thread_pool.thread.countto anObservableUpDownCounterwith unit{thread}.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Qyl.OpenTelemetry.AutoInstrumentation/QylRuntimeProcessMetrics.cs | Updates thread-pool thread count metric to semconv-correct instrument type and unit. |
| src/Qyl.OpenTelemetry.AutoInstrumentation/QylActivitySource.cs | Adds helper to start spans at ambient Activity start time to produce correct durations. |
| src/Qyl.OpenTelemetry.AutoInstrumentation.DiagnosticListeners/HttpClient/HttpClientDiagnosticListener.cs | Uses the new ambient-start helper so HttpClient spans have real durations. |
| src/Qyl.OpenTelemetry.AutoInstrumentation.DiagnosticListeners/GrpcClient/GrpcClientDiagnosticListener.cs | Uses the new ambient-start helper so gRPC client spans have real durations. |
| src/Qyl.OpenTelemetry.AutoInstrumentation.DiagnosticListeners/DiagnosticListenerSubscriber.cs | Updates documentation to accurately describe the DiagnosticListener stop-event bridge behavior. |
| src/Qyl.OpenTelemetry.AutoInstrumentation.DiagnosticListeners/AspNetCore/AspNetCoreDiagnosticListener.cs | Uses the new ambient-start helper so ASP.NET Core server spans have real durations. |
| var ambient = Activity.Current; | ||
| return ambient is null | ||
| ? Source.StartActivity(operationName, activityKind) | ||
| : Source.StartActivity(operationName, activityKind, ambient.Context, tags: null, links: null, startTime: ambient.StartTimeUtc); |
| /// primitive that's been AOT-safe since .NET 8, so this layer emits spans without any IL rewriting | ||
| /// or runtime code generation. Concrete subscribers react on the completion (<c>*.Stop</c>) event and | ||
| /// stamp the span to the ambient framework activity's start (via |
An adversarial OTel-compliance + honesty audit (45 agents, 39 candidates, 23 confirmed) rated qyl's OTel code 3/10. This is the verified subset of the root-library fixes.
*.Stopevent (StartTimeUtc = now) → every span had ~0 duration. NewQylActivitySource.StartAtAmbientStartstamps the span to the ambient framework Activity's real start (parented for correlation). All 3 listeners switched.DiagnosticListenerSubscriberclaimed it "publishes the same span shapes" (false while durations were fabricated). Doc now describes the real mechanism.dotnet.thread_pool.thread.countwas a unitlessObservableGauge; nowObservableUpDownCounter+ unit{thread}.Verified:
core.slnfRelease 0/0;verify-webapi-aot-demopasses. No PublicAPI change (all internal/private).Deliberately NOT here (separate verified increments — not faked): #3 double-count between the listener and interceptor lanes (+ the AOT fixture that masks it), #4/#11 OTLP Events/Links/Status.Message dropped on ingest (cross-repo: contract + DuckDB), #6 url.scheme, #7 method_original on the interceptor path, #9 Azure span name ignores methodName.
🤖 Generated with Claude Code