-
Notifications
You must be signed in to change notification settings - Fork 823
add request ID injection to context to enable tracking requests across services #6895
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?
Conversation
583fa70
to
d57c37c
Compare
…s downstream services Signed-off-by: Erlan Zholdubai uulu <[email protected]>
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!
require.Equal(t, providedID, requestID, "Request ID from header should be used") | ||
} | ||
|
||
func TestExistingRequestIdIsPreserved(t *testing.T) { |
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.
What's the goal here? Do you imagine a downstream component changing the header?
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.
No specific case—just a precaution to avoid changing the context if the value already exists. This shouldn't normally happen, but in case it does, it's safer not to override it.
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.
Thx for the logs from testing!
…queries Signed-off-by: Erlan Zholdubai uulu <[email protected]>
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.
The code lgtm, but it feels we are rewriting code from the log headers. The logs header is specific for logging, but one of the key parts was transferring ctx between services which seems to be duplicating here. I think we can reuse at least that logic
What if, we have a new config for the requestId header, but use the same middleware but adding the new header as targetHeader?
we can also refactor that util_log context function to a generic util.
would that also work? seems we would avoid some of this new changes
@@ -56,6 +56,7 @@ | |||
* [ENHANCEMENT] Distributor: Add native histograms max sample size bytes limit validation. #6834 | |||
* [ENHANCEMENT] Querier: Support caching parquet labels file in parquet queryable. #6835 | |||
* [ENHANCEMENT] Querier: Support query limits in parquet queryable. #6870 | |||
* [ENHANCEMENT] API: add request ID injection to context to enable tracking reqeusts across downstream services. #6895 |
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.
nit: s/reqeusts/requests
What this PR does:
This PR adds support for injecting a request ID into the request context to enable tracking requests across services.
This ID is propagated through the service call chain and enables future tracking of downstream operations tied to a single originating request.
Note: While there is no active usage of the request ID yet, the immediate goal is to use it for tracing storage-related resource usage, especially for identifying heavy or costly queries originating from a single logical request.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
Logs from testsing split queries got same request id