Set the request ID before the trace layer reads it - #819
Open
shaurya703 wants to merge 1 commit into
Open
Conversation
Fixes SO4-Markets#790. Chained Router::layer() calls make the LAST one added the outermost, so the request reached them bottom-up: track_metrics, trace_layer, SetRequestIdLayer, Propagate. make_span_with therefore read an extension SetRequestIdLayer had not written yet, and every span was created with request_id = "". SetRequestIdLayer now comes after trace_layer in the chain, which is what makes it run first. A comment above the chain spells the direction out, since reading it the natural way is exactly how this happened. The response header kept working throughout, because Propagate only needs the id by the time the response is built — which is why test_request_id_and_completion_logs passed while the feature was dead. The new test captures the span's own request_id field through a subscriber rather than the header, and asserts it is non-empty and equal to the header. Reading the field rather than parsing formatted output, so it does not depend on the log format staying JSON. Recorded via record_debug because the span uses `%request_id`, a Display value, which never reaches record_str; both are implemented so it does not quietly stop seeing the field. Verified against the old order: the new test fails with "every span carried an empty request_id" and the other three still pass, which is the point — they could not see this. 16 suites green, fmt and clippy -D warnings clean.
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.
Fixes #790. Also makes the README claim in #813 true, so that one can be closed or re-checked after this lands.
The fix
SetRequestIdLayernow comes aftertrace_layerin the chain, which is what makes it run first. ChainedRouter::layer()calls make the last one added the outermost, so the request reached them bottom-up andmake_span_withwas reading an extension that had not been written yet.I left a comment above the chain spelling the direction out, because reading it the natural way is exactly how this happened.
Why the existing test could not catch it
test_request_id_and_completion_logsasserts the response header, and the header never broke:PropagateRequestIdLayeronly needs the id by the time the response is built. So the feature was dead in the logs while its test stayed green — which is the part worth fixing properly, not just the ordering.The new test captures the span's own
request_idfield through a subscriber and asserts it is non-empty and equal to the header.Verified against the old order:
The other three passing under the broken order is the whole point.
Two implementation notes
record_debug, notrecord_str: the span usesrequest_id = %request_id, a Display value. My first version implemented onlyrecord_strand captured nothing — the test failed with "no request span was recorded at all", which is a false negative dressed as a failure. Both are implemented now so it cannot quietly stop seeing the field.There is also a floor assertion that at least one span was recorded, so an empty capture cannot pass for a check.
Checks
cargo test— 16 suites green.cargo fmt --checkandcargo clippy --all-targets -- -D warningsclean.