-
Notifications
You must be signed in to change notification settings - Fork 0
Feature: Add request_id in logs
#55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feature: Add request_id in logs
#55
Conversation
Co-authored-by: Copilot <[email protected]>
request_id/ correlation_id in logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03aeb04a09
ℹ️ 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".
| thread_local! { | ||
| /// Fallback storage for disconnected span scenarios. | ||
| /// Thread-bound, not task-bound; clear at request boundaries to prevent leakage. | ||
| static THREAD_REQUEST_CONTEXT: RefCell<Option<RequestContext>> = const { RefCell::new(None) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid thread-local fallback in async request handling
Using a thread-local fallback for request context means concurrent async requests that happen to run on the same worker thread will overwrite each other’s IDs. In a tokio-based Rocket server, multiple requests can interleave on one thread, so logs from request A can be tagged with request B’s request_id/correlation_id once B calls set_request_context. This is a regression from the prior span-only approach and will misattribute logs under load. Consider using task-local storage or only span extensions for async servers so context stays per-request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, switched to a task-local storage keyed by tokio::task::Id instead of thread-local. Span extensions remain the primary lookup, task-local is just a fallback for logs that lose span context.
GTC6244
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, Abhi! Couple of comments that I may be wrong about - hoping this is the case! ;-)
| extract_correlation_id(req).unwrap_or_else(|| format!("LD-{}", Uuid::new_v4())); | ||
| let (request_id, correlation_id) = extract_request_and_correlation_ids(req); | ||
|
|
||
| // Generate fallback ID only if BOTH headers are missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an anticipated situation? I ask, because each node will create a different one, and we'll only be able to trace what's happening on a single node this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i don't think we want a fallback ID. leave it empty if there's no ID parsed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this entirely per Chris's feedback. If no header is provided, we leave it empty.
|
|
||
| use crate::error::{EC, Error, Result, conversion_err_code, validation_err_code}; | ||
|
|
||
| pub const HEADER_KEY_X_CORRELATION_ID: &str = "X-Correlation-Id"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I can tell, X-Correlation-Id is just used for backwards compatibility, and the JS SDK stuffs in the request-id value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i believe the JS SDK uses X-Request-Id. code is here: https://github.com/LIT-Protocol/js-sdk/blob/6832ff6255f1320946cfba270eeb7e62fd341ba1/packages/lit-client/src/lib/LitNodeClient/LitNodeApi/src/helper/sendNodeRequest.ts#L42
we can standardize on X-Request-Id since that's what the client uses, or we can support both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind - i see you handle both header options below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we support both. X-Request-Id is preferred, but we fall back to X-Correlation-Id for older clients. I remember discussing this before.
| use rocket::fairing::{Fairing, Info, Kind}; | ||
| use rocket::{Data, Request, Response}; | ||
|
|
||
| /// Clears thread-local request context at request start and after response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this? I thought ( may be incorrectly ) that Rocket uses tokio async , and that thread-local storage didn't hold up under load. Rocket has a manager for this in Rocket.Request.Request cache, IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. Although I switched to task-local keyed by tokio::task::Id. Looked into Rocket's request cache but the tracing layer doesn't have access to the Request object? so went with task-local approach. Also, added a fairing to clear it at request boundaries. what do you think of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking maybe the other way - use rocket's cache from inside the fairing to sync the tracing context ... but you might have the same ( though inverse issue ). At the end of the day ( pardon the expression) if this works in a test environment, I'd give it a 👍 !
glitch003
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good - only change i would make is not to make up a fallback request id. if there's no ID parsed, just leave the field empty
|
|
||
| use crate::error::{EC, Error, Result, conversion_err_code, validation_err_code}; | ||
|
|
||
| pub const HEADER_KEY_X_CORRELATION_ID: &str = "X-Correlation-Id"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i believe the JS SDK uses X-Request-Id. code is here: https://github.com/LIT-Protocol/js-sdk/blob/6832ff6255f1320946cfba270eeb7e62fd341ba1/packages/lit-client/src/lib/LitNodeClient/LitNodeApi/src/helper/sendNodeRequest.ts#L42
we can standardize on X-Request-Id since that's what the client uses, or we can support both
| extract_correlation_id(req).unwrap_or_else(|| format!("LD-{}", Uuid::new_v4())); | ||
| let (request_id, correlation_id) = extract_request_and_correlation_ids(req); | ||
|
|
||
| // Generate fallback ID only if BOTH headers are missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i don't think we want a fallback ID. leave it empty if there's no ID parsed.
|
|
||
| use crate::error::{EC, Error, Result, conversion_err_code, validation_err_code}; | ||
|
|
||
| pub const HEADER_KEY_X_CORRELATION_ID: &str = "X-Correlation-Id"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind - i see you handle both header options below.
request_id/ correlation_id in logsrequest_id in logs
|
PASS [ 43.056s] (3/3) lit_node::test toxiproxy::perf_tests::load_with_no_latency |
GCP logs were missing request/correlation IDs, which made it hard to correlate logs with traces. PR implements the below:
Summary
request_idinto log recordsChanges