Skip to content
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

Propagate request ID through gRPC context #7356

Merged
merged 2 commits into from
May 15, 2024

Conversation

fpetkovski
Copy link
Contributor

The request ID only gets propagated through HTTP calls and is not available in gRPC servers.

This commit adds intereceptors to grpc servers and clients to make sure request ID propagation happens.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

The request ID only gets propagated through HTTP calls and is not available
in gRPC servers.

This commit adds intereceptors to grpc servers and clients to make sure request ID
propagation happens.

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
Copy link
Contributor

@pedro-stanaka pedro-stanaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Does it mean store gateway can log the request ID if it is set on the gRPC client side?

Copy link
Contributor

@MichaHoffmann MichaHoffmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@fpetkovski
Copy link
Contributor Author

fpetkovski commented May 15, 2024

Nice. Does it mean store gateway can log the request ID if it is set on the gRPC client side?

I need to double check how we can get this to the logger now. I think we use message tags right now (which works only locally), but we will have the request ID in the context and we can get it from there.

@yeya24
Copy link
Contributor

yeya24 commented May 15, 2024

Yeah I think the logger needs to be changed. Now the logger is the one in the bucket store instance and it doesn't take context into account so request ID cannot be propagated.

It would be nice to make the logger customizable using context so that Cortex can be benefited as well. As Cortex uses a different request ID format from Thanos so we need it to be a hook

@yeya24 yeya24 merged commit 9707a4f into thanos-io:main May 15, 2024
20 checks passed
@yeya24
Copy link
Contributor

yeya24 commented May 15, 2024

Let's merge this and we can go ahead and implement the logger hook

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants