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

Add tracing to the tenant federation mergeQueryable #4147

Closed
simonswine opened this issue Apr 29, 2021 · 2 comments · Fixed by #4186
Closed

Add tracing to the tenant federation mergeQueryable #4147

simonswine opened this issue Apr 29, 2021 · 2 comments · Fixed by #4186

Comments

@simonswine
Copy link
Contributor

simonswine commented Apr 29, 2021

This is a work item that came out the review of #4117

The mergeQueryable aggregates data from underlying queryables concurrently. Understanding the call sequence is only possible providing spans.

/assign @simonswine

Some examples that creates spans:

spanLog, spanCtx := spanlogger.New(q.ctx, "blocksStoreQuerier.LabelNames")

tracer := opentracing.GlobalTracer()

@pstibrany
Copy link
Contributor

Your first linked example is what I would suggest to use… getting spanlogger from existing context, closing it at the end of method, and passing new context to the called methods is a good start. Additionally logging supplied tenantIDs via new spanlogger (to make them visible when viewing the trace) would be useful.

We can go even further and create one child span per processed tenant – that can be useful for people troubleshooting slow queries to understand if there is specific tenant that was slow for some reason.

vlad-diachenko added a commit to vlad-diachenko/cortex that referenced this issue May 14, 2021
1. added logic to add tag `tenant_id` to tracing spans if tenant is defined in context.
2. added additional tracing spans to merge_querier's methods

Signed-off-by: Vladyslav Diachenko <[email protected]>
@vlad-diachenko
Copy link
Contributor

I have implemented a logic to add tenant_id tag to each tracing span if tenant_id is defined in context.
Also, I have added a few additional spans to mergeQuerier:

  • mergeQuerier.select
  • mergeQuerier.labelValues
  • mergeQuerier.labelNames

vlad-diachenko added a commit to vlad-diachenko/cortex that referenced this issue May 17, 2021
vlad-diachenko added a commit to vlad-diachenko/cortex that referenced this issue May 18, 2021
1.changed value of tenant_id tag in tracing span from string value to array
2. removed import alias for spanlogger

Signed-off-by: Vladyslav Diachenko <[email protected]>
vlad-diachenko added a commit to vlad-diachenko/cortex that referenced this issue May 20, 2021
1. fixed tests
2. fixed name of the spans in mergeQuerier

Signed-off-by: Vladyslav Diachenko <[email protected]>
vlad-diachenko added a commit to vlad-diachenko/cortex that referenced this issue May 20, 2021
Signed-off-by: Vladyslav Diachenko <[email protected]>
vlad-diachenko added a commit to vlad-diachenko/cortex that referenced this issue May 20, 2021
…gSlice function to create inner spans under this context

Signed-off-by: Vladyslav Diachenko <[email protected]>
vlad-diachenko added a commit to vlad-diachenko/cortex that referenced this issue May 20, 2021
vlad-diachenko added a commit to vlad-diachenko/cortex that referenced this issue May 20, 2021
…am that is not used in callback function

Signed-off-by: Vladyslav Diachenko <[email protected]>
vlad-diachenko added a commit to vlad-diachenko/cortex that referenced this issue May 20, 2021
Signed-off-by: Vladyslav Diachenko <[email protected]>
pracucci pushed a commit that referenced this issue May 20, 2021
* #4147
1. added logic to add tag `tenant_id` to tracing spans if tenant is defined in context.
2. added additional tracing spans to merge_querier's methods

Signed-off-by: Vladyslav Diachenko <[email protected]>

* #4147 fixed errors reported by linters

Signed-off-by: Vladyslav Diachenko <[email protected]>

* #4147
1.changed value of tenant_id tag in tracing span from string value to array
2. removed import alias for spanlogger

Signed-off-by: Vladyslav Diachenko <[email protected]>

* #4147
1. fixed tests
2. fixed name of the spans in mergeQuerier

Signed-off-by: Vladyslav Diachenko <[email protected]>

* #4147 updated CHANGELOG.md

Signed-off-by: Vladyslav Diachenko <[email protected]>

* #4147 passed method's span context to mergeDistinctStringSlice function to create inner spans under this context

Signed-off-by: Vladyslav Diachenko <[email protected]>

* #4147 added mocktracer package for testing purpose

Signed-off-by: Vladyslav Diachenko <[email protected]>

* #4147 fixed code style and removed redundant context param that is not used in callback function

Signed-off-by: Vladyslav Diachenko <[email protected]>

* #4147 fixed code style

Signed-off-by: Vladyslav Diachenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants