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

Receiver: instrument multitsdb tenant series tracing tag correctly #5002

Closed
yeya24 opened this issue Dec 26, 2021 · 5 comments · Fixed by #5148
Closed

Receiver: instrument multitsdb tenant series tracing tag correctly #5002

yeya24 opened this issue Dec 26, 2021 · 5 comments · Fixed by #5148

Comments

@yeya24
Copy link
Contributor

yeya24 commented Dec 26, 2021

At https://github.com/thanos-io/thanos/blob/main/pkg/store/multitsdb.go#L274, a tenant tag is added. But it uses ClientAddContextTags method and that method is only used in gRPC method calls. In the case of tenant's local TSDB, there is no gRPC call anymore so the instrumentation doesn't work.

I expect to see https://github.com/thanos-io/thanos/blob/main/pkg/store/multitsdb.go#L172 get properly instrumented with the tenant tag.

Screenshot of the traces for tenant series call:
image

@wiardvanrij
Copy link
Member

Added tag good first issue - even though this is somewhat deeper into the tech, I think with the comment from @yeya24 it should be a fun thing to pick up. Feel free to ask help if needed.

@metonymic-smokey
Copy link
Contributor

From a quick look at the linked code references, I think adding a tag to the context here should fix it?

@yeya24
Copy link
Contributor Author

yeya24 commented Jan 9, 2022

From a quick look at the linked code references, I think adding a tag to the context here should fix it?

Yeah I think so, would you like to give it a try?

@metonymic-smokey
Copy link
Contributor

metonymic-smokey commented Jan 10, 2022 via email

@metonymic-smokey
Copy link
Contributor

Opened a draft PR. Please let me know if I'm proceeding in the right direction. Many thanks!

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

Successfully merging a pull request may close this issue.

3 participants