feat(frontend): join upstream W3C trace context into engine request spans - #791
feat(frontend): join upstream W3C trace context into engine request spans#791xiaguan wants to merge 20 commits into
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: 63704c73bc
ℹ️ 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".
| .remove(request_id) | ||
| .map(|(traceparent, _)| traceparent) |
There was a problem hiding this comment.
When a traced request is rejected before reaching the bridge, its stash entry can survive indefinitely because expiry cleanup only runs during an insertion at full capacity, while take returns the value without checking its timestamp. If a later request reuses that X-Request-Id without a traceparent—for example, a retry or a request to the other API route—the bridge consumes the stale parent and incorrectly attaches the new request to the old trace. Check the stored timestamp on retrieval (or otherwise expire entries independently of reaching capacity).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb20b34 — take now drops entries older than TTL instead of returning them (regression test take_drops_expired_entries).
…pans The pinned vllm-server forwards X-Request-Id but drops traceparent, so behind an OTel-speaking proxy (e.g. vllm-router --enable-trace) every engine request opened a fresh trace and the e2e chain split at the frontend boundary. Stash the incoming traceparent at the axum boundary keyed by the request's X-Request-Id (generated and injected when absent), and let the engine bridge pop it via external_req_id — tolerating vllm-server's cmpl-/chatcmpl- prefixes — as the parent of the request root span. Falls back to a random context when no upstream traceparent exists. Local workaround until vllm-project/vllm#50370 (HTTP-layer trace_headers population) merges and the pin is bumped; migration tracked in #790. Signed-off-by: xiaguan <751080330@qq.com>
Grafana 11.3 denies anonymous Viewers datasources:explore (Access denied in the server log; Explore renders but returns nothing), which contradicted this file's own usage comment. Switch the anonymous role to Editor — the port stays loopback-only — and note the OTel-router usage that joins client → router → openinfer → prefill/decode in one trace. Signed-off-by: xiaguan <751080330@qq.com>
Single-trace verification of client → vllm-router → openinfer → prefill/decode on RTX 5070 Ti (both OpenAI endpoints, with/without client traceparent, 16-way concurrency), the cmpl- prefix correlation pitfall, Tempo query caveats, and the upstream migration plan (vllm#50370, #790). Signed-off-by: xiaguan <751080330@qq.com>
Signed-off-by: xiaguan <751080330@qq.com>
Also restores the intended section order (Preparation → Execution Log → Debrief) that the appended decision block had broken. Signed-off-by: xiaguan <751080330@qq.com>
Codex review on #791: take() returned entries without checking their timestamp, so a request reusing an X-Request-Id could consume a stale traceparent left behind by a request rejected before reaching the engine, attaching the new request to an old trace. Drop expired entries on take instead of returning them. Signed-off-by: xiaguan <751080330@qq.com>
Signed-off-by: xiaguan <751080330@qq.com>
45bc37f to
2e760ff
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e760ff872
ℹ️ 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".
| else { | ||
| return; |
There was a problem hiding this comment.
When a traced request is rejected before reaching the bridge and an immediate retry reuses its X-Request-Id without traceparent, this early return leaves the first request's stash entry intact; the retry's bridge lookup then consumes it and attaches the retry to the old trace. The fresh evidence in this revision is that take now explicitly returns every entry younger than 120 seconds, so the TTL fix only bounds this misattribution and does not prevent the normal within-TTL retry case. Requests without trace context should invalidate any pending entry for their correlation key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5ea8017 — requests carrying no trace context now invalidate any pending stash entry under their X-Request-Id, so an untraced retry can no longer join the previous attempt trace (regression test untraced_retry_invalidates_pending_entry).
Second Codex finding on #791: the TTL-on-take fix only bounds the misattribution window. A request rejected before reaching the engine leaves its stash entry behind, and an immediate retry reusing the X-Request-Id without a traceparent would still consume it at the bridge and join the previous attempt's trace. Requests that carry no trace context now invalidate any pending entry under their id. Signed-off-by: xiaguan <751080330@qq.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ea8017f15
ℹ️ 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".
| /// known prefixes stripped (longest first — `cmpl-` is a suffix of | ||
| /// `chatcmpl-`'s tail). | ||
| pub(crate) fn take_for_external_req_id(&self, external_req_id: &str) -> Option<String> { | ||
| self.take(external_req_id).or_else(|| { |
There was a problem hiding this comment.
When concurrent completion requests use distinct valid X-Request-Id values such as foo and cmpl-foo, the first request reaches the bridge as cmpl-foo, and this exact lookup consumes the second request's stashed parent before trying the prefix-stripped key foo. The first request is consequently attached to the wrong trace and the second starts a new trace; use an unambiguous, route-aware canonical key rather than exact-before-stripped fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bcede4a — lookup now strips exactly one route prefix first (vllm-server prepends exactly one on these routes), with exact-id only as the fallback for unprefixed routes; regression test strip_lookup_wins_over_exact_on_prefix_collision covers the foo / cmpl-foo collision.
Third Codex finding on #791: exact-before-stripped lookup let a request with header foo (bridge id cmpl-foo) steal a concurrent request whose header is literally cmpl-foo. vllm-server prepends exactly one route prefix, so strip one known prefix and look that up first; the exact id is only a fallback for routes that prepend nothing. Signed-off-by: xiaguan <751080330@qq.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcede4a4b6
ℹ️ 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".
| inner.clear(); | ||
| } | ||
| } | ||
| inner.insert( |
There was a problem hiding this comment.
When overlapping requests or retry attempts reuse the same caller-controlled X-Request-Id but carry different trace parents, this insertion overwrites the first pending entry. The first matching EngineCoreRequest then consumes whichever parent was inserted last, while the other request starts a random trace, so hedged retries or concurrent clients can be assigned to the wrong trace. Use a correlation key that is unique per HTTP attempt rather than a single map value per external ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3dfba61 — entries now queue FIFO per X-Request-Id, so concurrent duplicate ids (hedged retries) each consume their own parent; a per-attempt unique key is unavailable since external_req_id is the only correlation the bridge can derive. Regression test duplicate_ids_keep_separate_parents_fifo.
| - Unit tests: middleware injection/no-header paths + bridge parent resolution (TestReporter pattern from phase_trace.rs) | ||
| 2. Bring up the local stack: `docker compose -f deploy/tracing/docker-compose.yml up -d` (Tempo 4317 / Grafana 3000) | ||
| 3. Start openinfer: `OPENINFER_TRACE_OTLP_ENDPOINT=http://127.0.0.1:4317 cargo run --release -- --model-path models/Qwen3-4B --port 8000` (confirm weights and GPU first) | ||
| 4. Build and start the router (/data/code/workspace-rustllm/router): `cargo build --release`, then `vllm-router --worker-urls http://127.0.0.1:8000 --port 8090 --enable-trace --otlp-traces-endpoint <4317>` (verify flag format at execution) |
There was a problem hiding this comment.
When a reader copies this documented command, Bash interprets <4317> as input redirection from a file named 4317, so the router does not start. Replace it with the verified endpoint shown later in the execution record, such as 127.0.0.1:4317; repository guidance requires every documented command to have been run and verified before commit.
AGENTS.md reference: AGENTS.md:L135-L136
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3dfba61 — the <4317> placeholder is now the verified 127.0.0.1:4317 endpoint.
CI's newer clippy (-D warnings, --all-targets) rejects Instant arithmetic the local toolchain accepts; use checked_sub in the test fixture. Signed-off-by: xiaguan <751080330@qq.com>
Fourth Codex finding on #791: a second insert under the same X-Request-Id overwrote the pending entry, so concurrent attempts reusing one id (hedged retries) could consume each other's parent. Entries now queue FIFO per id, each popped at most once; a per-attempt unique key is unavailable because external_req_id is the only correlation the bridge can derive downstream. Also factors the nested stash type behind an alias for clippy::type_complexity. Bundled doc fix (second finding): replace the <4317> router endpoint placeholder in the tracing doc with the verified 127.0.0.1:4317 — angle brackets read as shell input redirection when copied, and AGENTS.md requires documented commands to be run and verified. Signed-off-by: xiaguan <751080330@qq.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dfba61e69
ℹ️ 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".
| while let Some((traceparent, inserted)) = queue.pop_front() { | ||
| if inserted.elapsed() < TTL { | ||
| result = Some(traceparent); | ||
| break; |
There was a problem hiding this comment.
When a traced request is rejected before producing an EngineCoreRequest and a retry reuses the same X-Request-Id with a new traceparent within 120 seconds, insert appends the retry's parent but this FIFO pop returns the rejected attempt's still-live parent; the retry therefore joins the wrong trace and leaves its own parent queued for a future request. The fresh evidence after the earlier untraced-retry fix is that invalidation only runs when traceparent is absent, so a traced retry still follows this push_back/pop_front path. Correlate individual attempts or remove the abandoned entry before consuming the retry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 907425c — the middleware now watches the response: an error status means the request never reached the engine, so its just-stashed entry is discarded immediately (discard_oldest, FIFO-fair across duplicates). A traced retry within the TTL now pops its own parent; regression test traced_retry_after_rejection_gets_fresh_parent.
Fifth Codex finding on #791: a traced request rejected before reaching the engine left its entry queued, so a traced retry reusing the X-Request-Id within the TTL popped the abandoned attempt's parent instead of its own. The middleware now watches the response: an error status means the request never reached the engine, and its just-stashed entry is discarded immediately (discard_oldest, FIFO-fair for duplicates). Invalidation on untraced requests is kept for the no-traceparent retry path. Signed-off-by: xiaguan <751080330@qq.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 907425c402
ℹ️ 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".
| if !response.status().is_success() { | ||
| if let Some(id) = stashed_id { | ||
| stash.discard_oldest(&id); | ||
| } |
There was a problem hiding this comment.
When two overlapping traced requests share an X-Request-Id, the later request can finish HTTP validation with an error before the earlier request reaches the bridge. This call then removes the queue head belonging to the earlier request, so that request consumes the rejected request's parent and joins the wrong trace. The fresh evidence in this revision is the new post-response discard_oldest cleanup; retain an insertion token and remove that specific entry instead of assuming responses complete in FIFO order.
Useful? React with 👍 / 👎.
| let stashed_id = stash_from_headers(&stash, request.headers_mut()); | ||
| let response = next.run(request).await; | ||
| if !response.status().is_success() { | ||
| if let Some(id) = stashed_id { | ||
| stash.discard_oldest(&id); |
There was a problem hiding this comment.
When a traced successful endpoint such as /metrics or /v1/models passes through this globally mounted middleware, it creates a stash entry but never produces an EngineCoreRequest, and the success-status condition leaves that entry live for 120 seconds. Sufficient traced probes fill the 4096-entry capacity and trigger inner.clear(), which can discard parents for active inference requests and split their traces; limit stashing to generation routes or remove an entry when the response completes without bridge consumption.
Useful? React with 👍 / 👎.
Sixth Codex round on #791, two findings: - discard_oldest assumed responses complete in FIFO order: when two overlapping traced requests shared an id and the later one errored first, the head entry (the still-live earlier attempt's parent) was dropped instead. Insertions now carry a per-attempt token and the error path discards exactly its own entry (discard_entry). - The globally mounted middleware stashed traceparents for non-engine routes (/metrics, /v1/models, ...), whose entries never reach the bridge and could fill the stash until the capacity clear split live traces. Stashing is now limited to POST /v1/completions, /v1/chat/completions and /inference/v1/generate — the routes whose accepted requests produce an EngineCoreRequest. Signed-off-by: xiaguan <751080330@qq.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7da3d40c0
ℹ️ 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".
| if let Some(bare) = bare { | ||
| return self.take(bare); |
There was a problem hiding this comment.
When /inference/v1/generate receives an X-Request-Id such as cmpl-foo, its unprefixed external_req_id is also cmpl-foo, but this branch treats it as a completions-generated prefix and only looks up foo. The request therefore starts a new trace while the actual cmpl-foo entry remains until TTL expiry; if foo is also pending, it can consume that unrelated parent instead. Make canonicalization route-aware rather than inferring the route solely from the ID prefix.
Useful? React with 👍 / 👎.
| if let Some(id) = request_id { | ||
| stash.invalidate(&id); |
There was a problem hiding this comment.
When an untraced request overlaps a traced request with the same caller-controlled X-Request-Id before the traced request reaches the bridge, this invalidates the entire queue and removes the traced request's still-needed parent. The fresh evidence in this revision is that duplicate IDs are now explicitly queued, but the no-header path still deletes every queued attempt; enqueue a per-attempt untraced marker instead so each downstream request consumes only its own slot.
Useful? React with 👍 / 👎.
Seventh Codex round on #791, two findings sharing one root cause — the middleware is the only place the route is known: - Prefix-stripped lookup guessed the route from the id shape, breaking /inference/v1/generate (which prepends nothing) for headers literally starting with cmpl-. The middleware now computes the correlation key up front (route prefix + X-Request-Id, mirroring vllm-server's completions/chat prefixing) and the bridge does a plain exact lookup; the strip-first/exact-fallback heuristics are gone. - An untraced request invalidated the whole queue for its id, deleting an overlapping traced attempt's still-needed parent. Untraced attempts now queue a None marker reserving their own slot: the bridge pops it and opens a fresh trace for exactly that attempt, leaving every other attempt's parent intact. Signed-off-by: xiaguan <751080330@qq.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22645de425
ℹ️ 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".
| let parent = external_req_id | ||
| .as_deref() | ||
| .and_then(|id| self.trace_stash.take(id)) |
There was a problem hiding this comment.
For a traced streaming request rejected by the bridge before this point—such as one with a nonzero frequency_penalty, presence_penalty, or unsupported seed—the early returns above emit a terminal engine error without consuming the stash entry. Because a streaming response can already have a successful HTTP status, the middleware's non-success cleanup does not remove it; a later request reusing the same X-Request-Id then consumes the rejected request's parent and joins the wrong trace. Take or explicitly discard the entry as soon as the EngineCoreRequest reaches the bridge, before these validation returns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e13343b — start_request now takes the stash entry the moment the EngineCoreRequest arrives, before all validation early-returns, so bridge-side rejections retire the entry instead of leaking it past the middleware status check (streaming responses can be 200).
Eighth Codex finding on #791: bridge-side validation rejections (missing prompt/sampling params, unsupported seed or penalties, bad LoRA params) terminate the request before span creation, never consuming the stash entry — and streaming clients still get a success status, so the middleware's error-path cleanup cannot cover them. The entry then leaked until the TTL, letting a within-TTL reuse of the same X-Request-Id join the rejected request's trace. start_request now takes the entry the moment the EngineCoreRequest arrives; every terminal path retires it. Signed-off-by: xiaguan <751080330@qq.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e13343b59f
ℹ️ 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".
| let (result, queue_empty) = { | ||
| let queue = inner.get_mut(request_id)?; | ||
| let mut result = None; | ||
| while let Some((_, traceparent, inserted)) = queue.pop_front() { |
There was a problem hiding this comment.
When concurrent requests reuse an X-Request-Id but carry different trace parents, their downstream order can differ from their intake order; for example, a first LoRA request can remain in body buffering/rewriting in forward_lora_openai_request while a later, smaller request reaches the bridge first. This unconditional pop_front then attaches the later request to the first request's parent and vice versa. The fresh evidence beyond the earlier duplicate-ID discussion is that the queue preserves middleware intake order, while the repository's asynchronous LoRA forwarding path can reorder bridge arrival. Correlate each attempt uniquely or serialize duplicate IDs until their corresponding slot is consumed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged in 7e9e038 with the ceiling documented at the mechanism. Reordering is real (your LoRA body-rewriting example), but exact pairing for concurrent requests sharing one X-Request-Id is impossible by construction: the correlation key (external_req_id) is identical for both attempts and the engine request_id suffix is random with no intake information. The middleware intake timestamp and vllm-server arrival_time are the only order signals available, and matching on them would be a heuristic with false precision. FIFO preserves per-attempt slots and pairs correctly in the common (non-reordered) case; callers needing deterministic pairing must use unique request ids — X-Request-Id is contractually unique per request.
Ninth Codex round on #791: downstream order can differ from intake order for concurrent requests sharing an X-Request-Id (e.g. a LoRA request stalled in body rewriting while a later request overtakes it), so FIFO pairing can attach such attempts to each other's parents. Exact pairing is impossible by construction — the correlation key is identical and the engine request_id's random suffix carries no intake information — so document the ceiling at the mechanism rather than pretend a deterministic order; callers needing exact pairing must use unique request ids. Signed-off-by: xiaguan <751080330@qq.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e9e038b61
ℹ️ 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".
| let stashed = stash_from_headers(&stash, &path, request.headers_mut()); | ||
| let response = next.run(request).await; |
There was a problem hiding this comment.
When a client disconnects while the downstream handler is still buffering or validating the request body, Axum can drop this middleware future before next.run(request).await returns, so the post-response cleanup never executes and no EngineCoreRequest exists to consume the entry. A retry within 120 seconds that reuses the same X-Request-Id can then consume the abandoned parent and join the cancelled attempt's trace; retain a drop guard that discards this insertion unless the bridge has already consumed it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6957687 — an InsertionGuard now retires the stashed entry whenever the middleware future ends (normal completion, error response, or cancellation on client disconnect). The bridge consumes entries at engine intake for every request it sees, so the guard only ever removes entries whose request never reached the engine; it also subsumes the error-status cleanup. Regression test dropped_guard_retires_only_its_own_entry.
Tenth Codex finding on #791: a client disconnect while the handler is still buffering or validating the body drops the middleware future before the post-response code runs, leaking the entry until the TTL and letting a within-TTL retry join the cancelled attempt's trace. An InsertionGuard now retires the insertion when the future ends for any reason — normal completion, error response, or cancellation. The guard is a no-op whenever the bridge already consumed the entry at engine intake, so it also subsumes the error-status cleanup. Signed-off-by: xiaguan <751080330@qq.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69576873d0
ℹ️ 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".
| token, | ||
| } | ||
| }); | ||
| next.run(request).await |
There was a problem hiding this comment.
When a generation request uses streaming, next.run(request).await can return as soon as the response body is created, while the asynchronously sent EngineCoreRequest is still waiting to be processed by the bridge. Returning here immediately drops _guard, which removes the stash entry; if the bridge runs afterward, take misses the upstream parent and starts a separate trace. Disarm the guard when a successful response is returned so the bridge can consume the entry, while retaining drop cleanup for cancellation and pre-engine error paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e73b6f4 — the guard is now disarmed on successful responses (the engine request is necessarily sent before the response is created; the bridge consumes the entry when it processes it), and still fires on error statuses and on future cancellation. Regression test disarmed_guard_leaves_entry_for_bridge_consumption.
Eleventh Codex finding on #791: for streaming requests the response head can go out while the asynchronously sent EngineCoreRequest is still waiting for the bridge, so dropping the guard at future end deleted the parent before the bridge could consume it. The guard is now disarmed on successful responses (engine request already sent; bridge consumes the entry when it processes it), and still fires on error statuses and on cancellation. Signed-off-by: xiaguan <751080330@qq.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e73b6f4d9a
ℹ️ 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".
| while let Some((_, traceparent, inserted)) = queue.pop_front() { | ||
| if inserted.elapsed() < TTL { | ||
| result = traceparent; | ||
| break; |
There was a problem hiding this comment.
When two requests reuse an X-Request-Id and the first spends longer than 120 seconds before reaching the bridge—for example, while its body is uploaded or rewritten—the first bridge lookup discards its expired slot and continues to consume the second request's still-live parent. Even if the requests then reach the bridge in intake order, the first joins the second trace and the second gets no parent. Stop after consuming the first queue slot when it is expired rather than scanning into later attempts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5ffcafd — take() now consumes exactly one slot per bridge arrival: an expired head belongs to this arrival attempt and yields a fresh trace instead of sliding into the next attempt live parent. Regression test expired_head_slot_does_not_slide_into_next_attempt.
Twelfth Codex finding on #791: take() scanned past an expired queue head into the next attempt's live parent, so a request whose first attempt outlived the TTL (e.g. a long body upload) joined the second attempt's trace and orphaned it. Slots pair attempts to bridge arrivals in order, so an arrival now consumes exactly its own slot: fresh parent joins, expired slot or untraced marker starts a fresh trace, and later attempts keep theirs. Signed-off-by: xiaguan <751080330@qq.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ffcafda6a
ℹ️ 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".
| external_req_id | ||
| .as_deref() | ||
| .and_then(|id| self.trace_stash.take(id)) |
There was a problem hiding this comment.
When one /v1/completions call expands into multiple EngineCoreRequests, such as a batched prompt or n > 1, those children share the HTTP request's external_req_id, but this one-shot take removes its only stashed parent for the first child. Later children therefore start unrelated traces; if another overlapping request reuses that ID, a child can instead consume that request's queued parent and misalign both traces. Retain or replicate the parent until every engine request produced by the HTTP attempt has been correlated.
Useful? React with 👍 / 👎.
Signed-off-by: xiaguan <751080330@qq.com>
|
This pull request has been inactive for 14 days. It will be closed after another 30 days unless there is new activity. |
1 participant
Summary
When openinfer serves behind an OTel-speaking proxy (e.g. vllm-project/router with
--enable-trace), the router injectstraceparentdownstream, but the pinned vllm-server (Rust frontend, git dep) forwards onlyX-Request-Idand drops it — so the bridge always opened a fresh trace (SpanContext::random()) andclient → router → openinfer → prefill → decodesplit into two disconnected traces.This PR closes the last hop without touching the git dependency:
openinfer-vllm-frontend/src/trace_context.rs(new): axum middleware (wired via the existingserve_with_router_extensionhook) stashes the incomingtraceparent, keyed by the request'sX-Request-Id(generated + injected when absent). Bounded stash (TTL 120s / cap 4096, one-shot pop).bridge.rs: onEngineCoreRequest, pop the stash byexternal_req_id— tolerating vllm-server'scmpl-/chatcmpl-prefixes — decode the W3C traceparent, and use it as the parent of therequestroot span. Falls back to a random context when absent/invalid; a non-sampled upstream context is honored (no spans collected). Zero stash work when tracing is disabled.deploy/tracing/docker-compose.yml: anonymous role Viewer → Editor — Grafana 11.3 denies anonymous Viewersdatasources:explore(Access denied in the server log; Explore rendered but returned nothing), contradicting the file's own usage comment. Port stays loopback-only. Also documents the router-chain usage.docs/subsystems/tracing/e2e-router-tracing.md: full record — mechanism, pitfalls, load numbers, migration plan.This is a deliberate local workaround. The proper fix is upstream: vllm-project/vllm#50370 populates
EngineCoreRequest.trace_headersin the Rust server (Python parity). Once it merges and the pin is bumped, the bridge readstrace_headers["traceparent"]directly and the middleware is deleted — migration tracked in #790.Test plan
cargo test --release -p openinfer-vllm-frontend --lib— 31/31 (4 new: stash roundtrip + W3C decode, id injection, no-header no-op,cmpl-/chatcmpl-prefix tolerance). Clippy clean.http_request→ routerhttp_client_request→ openinferrequest→queue/prefill/decode, for both/v1/completionsand/v1/chat/completions, with a client-supplied traceparent and router-rooted without one.Fixes partially (the local-workaround half of) #790.