[codex] Rebuild main as slim semantic history - #2
Conversation
![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: 34257074fb
ℹ️ 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".
|
|
||
| public static void SetUrlTags(Activity? activity, string? url, string? serverAddress, int? serverPort) | ||
| { | ||
| SemanticTagWriter.Set(activity, SemanticAttributes.UrlFull, url); |
There was a problem hiding this comment.
When the Hosting package is used with default options, the HttpClient diagnostic-listener path reads url.full/http.url from the framework payload or Activity.Current and writes it directly to the qyl span. That bypasses the documented QYL_AUTOINSTRUMENTATION_CAPTURE_SENSITIVE_VALUES gate, so an ordinary outgoing request like https://api.example/?access_token=... leaks the full query string into exported telemetry even though sensitive values are supposed to be off by default.
Useful? React with 👍 / 👎.
| SemanticTagWriter.Set(activity, SemanticAttributes.HttpRequestMethod, method); | ||
| SemanticTagWriter.Set(activity, SemanticAttributes.HttpRequestMethodOriginal, originalMethod); | ||
| SemanticTagWriter.Set(activity, SemanticAttributes.HttpRoute, route); | ||
| SemanticTagWriter.Set(activity, SemanticAttributes.UrlPath, path); |
There was a problem hiding this comment.
For ASP.NET Core diagnostic-listener spans, path is taken from HttpContext.Request.Path and always emitted as url.path. With the default CaptureSensitiveValues=false, this still exports raw user paths such as /users/alice/reset-token/..., contradicting the documented privacy rule that url.path is emitted only after explicitly opting in to sensitive values.
Useful? React with 👍 / 👎.
| public static Task<HttpResponseMessage> GetAsync(HttpClient client, string? requestUri) | ||
| { | ||
| ThrowIfNullClient(client); | ||
| var observation = StartHttpClientObservation(QylSemanticAttributes.HttpRequestMethodGet, requestUri); |
There was a problem hiding this comment.
When the source-generated HttpClient path sees the common BaseAddress pattern, e.g. new HttpClient { BaseAddress = ... }.GetAsync("/probe"), the observation is started from only the relative requestUri before HttpClient combines it with BaseAddress. Because the URI is not absolute here, the span misses server.address/server.port and any absolute url.full, so source-generated HTTP client telemetry is incomplete for relative requests.
Useful? React with 👍 / 👎.
| private static readonly Meter Meter = new(QylMetricMeters.HttpClientMeterName); | ||
| private static readonly Histogram<double> RequestDuration = Meter.CreateHistogram<double>(QylMetricNames.HttpClientRequestDuration, "s"); |
There was a problem hiding this comment.
When HTTPCLIENT metrics are enabled, QylMetricMeters.GetEnabledMeterNames() adds the framework System.Net.Http meter to the provider, while this helper also creates its own http.client.request.duration histogram on that same meter name. For source-visible HttpClient calls this records a qyl duration in addition to the BCL duration with the same meter/instrument identity, so metric exports contain duplicate indistinguishable HTTP client duration streams.
Useful? React with 👍 / 👎.
…sets The slim-history rebuild of main (PR #2) orphaned the original working branch, which then received nine more commits (2026-06-09 → 06-11 04:02) that never reached the new main: HttpClient effective-request metadata resolution, header-capture fallback path (new QylCaptureHelpers), status-handling tightening, verifier simplification into shared verify_helpers.py, and the extracted WebApiAotDemo Program.cs that the rebuild had dropped while keeping its golden report. Three-way merged the two genuinely diverged files: ModuleInitializerBoot (night listener-table refactor + main's sober XML docs) and the source generator (night dispatch-table refactor + main's ANcpLua.Roslyn.Utilities adoption). Skipped docs/pr1-remaining.md — completed-checklist evidence for the already-merged PR #1; its tracked changes land with this commit. Also filed the drafted NServiceBus AOT upstream issue under docs/upstream/ instead of leaving it untracked at the repo root. Verified: dotnet build (0 warnings), verify-generator-snapshots, verify-public-api-baseline, verify-source-interceptor-consumer all ok. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…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>
1 participant
What this PR is
This is a review artifact for replacing the current 193-commit
mainhistory with a 5-commit semantic history.It is intentionally opened against
codex/slim-main-history-base, an empty frozen base branch. GitHub PRs cannot rewrite already-publishedmainhistory. If this replacement history is accepted, the actual destructive operation is a later force-push ofcodex/slim-main-historytomain.Current-state evidence
origin/mainhas 193 commits.mainas merge commit8a92ba4ecdbe7e1e8f6ef83da607e67e043e5976.origin/main.a549f9dc0f70ce9243d19d5df762902053a94186to9a4c2f37bda2c15b3387ed709a506adb58f08d4a.Replacement history
This branch rebuilds the repository into 5 commits:
Establish repository foundationImplement AOT autoinstrumentation runtimeAdd real consumer demosAdd behavioral verification gatesDocument runtime semantics and coverageValidation
Locally run on
codex/slim-main-history:git diff --exit-code origin/main -- .python3 tools/verify-aot-autoinstrumentation-goal.pyResult:
aot-autoinstrumentation-goal-ok.Important operational note
Do not merge this PR into the empty base as a normal product change. Review it as the proposed replacement history. The destructive follow-up, if approved, is: