#4147 added `tenant_id` tag to tracing spans #4186

vlad-diachenko · 2021-05-14T16:18:24Z

What this PR does:

added logic to add tag tenant_ids to tracing spans if tenant is defined in context.
added additional tracing spans to merge_querier's methods

Which issue(s) this PR fixes:
Fixes #4147

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

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 <vlad.diachenko@grafana.com>

Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

simonswine

Thanks this looks already quite good. 👍

See my comments in-line and feel free to ping me when things are unclear

pkg/util/spanlogger/spanlogger.go

pkg/querier/tenantfederation/merge_queryable_test.go

simonswine · 2021-05-17T11:46:25Z

pkg/querier/tenantfederation/merge_queryable.go

+	// TODO: it's necessary to think how to override context inside querier
+	//  to mark spans created inside querier as child of a span created inside
+	//  methods of merged querier.


I think it would make sense to create a span during the creation of the queriers and the finish the span once Close() of the resulting mergeQuerier is called.

I have played with that here:

6ebd273

That's how that looks like:

This feels at least like it can group the same tenants nicely together. Obvisouly for a real fix we need context propagation as a part of LabelValues,LabelName, Select.

yes, it will help to group spans by tenant_id . thanks, i will implement it

however, it might confuse a little bit because all spans mergeQuerier.NewQuerier will take almost similar time for each tenant. but inside this span we will see real picture.
in current implementation it looks less confusing, i think.

what do you think?

To be honest I agree partly with your point, but I am not too sure, how to fix this properly.

I guess part of the problem is that the a query is only finished once Close() is called, which happens at the same time for underlying queriers.

I think the most confusing part of not grouping spans by tenant_id is seeing all (and even unrelated) spans together. I think until we implement context passing in those methods, we won't be able to solve that properly.

You could instantiate the queriers (call the callback inside LabelValues, etc.) and the right context will be applied.

simonswine · 2021-05-17T11:47:57Z

pkg/util/spanlogger/spanlogger.go

@@ -34,6 +39,9 @@ func New(ctx context.Context, method string, kvps ...interface{}) (*SpanLogger,
 // retrieved with FromContext or FromContextWithFallback.
 func NewWithLogger(ctx context.Context, l log.Logger, method string, kvps ...interface{}) (*SpanLogger, context.Context) {
 	span, ctx := opentracing.StartSpanFromContext(ctx, method)
+	if ids, _ := tenant.TenantIDs(ctx); ids != nil {


I am not too sure what other side effects this could have, but I think in general it is a good idea to always have the context's tenant_id(s) in the tracing span.

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 <vlad.diachenko@grafana.com>

pkg/querier/tenantfederation/merge_queryable.go

pkg/util/spanlogger/spanlogger.go

replay

LGTM, I only have minor comments

(note that I'm not a maintainer, so this approval doesn't count towards the required approval count)

1. fixed tests 2. fixed name of the spans in mergeQuerier Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

pkg/querier/tenantfederation/merge_queryable.go

gouthamve

LGTM, and I wonder how bad is creating the queriers inside Select, LabelValues, etc.

pkg/querier/tenantfederation/merge_queryable.go

pkg/util/spanlogger/spanlogger.go

…on to create inner spans under this context Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

simonswine

LGTM

I think there is still a go mod vendor necessary to make the CI/CD work

simonswine · 2021-05-20T10:11:40Z

I wonder how bad is creating the queriers inside Select, LabelValues, etc.

I think with Select it might be problematic, as it would have to Close() before the stoarge.SeriesSet is fully iterated through

Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

pracucci

LGTM modulo a couple of nits I would be glad to see fixed before merging. Thanks!

CHANGELOG.md

pkg/querier/tenantfederation/merge_queryable.go

…t used in callback function Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

pkg/querier/tenantfederation/merge_queryable.go

Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

pkg/util/spanlogger/spanlogger.go

pull-request-size bot added the size/L label May 14, 2021

#4147

3a3cb4b

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 <vlad.diachenko@grafana.com>

vlad-diachenko force-pushed the added_tenant_id_tag_to_tracing_span branch from 5b465ff to 3a3cb4b Compare May 14, 2021 18:17

vlad-diachenko mentioned this pull request May 16, 2021

wrong tracing spans hierarchy in mergeQuerier meethods #4189

Closed

2 tasks

#4147 fixed errors reported by linters

acef697

Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

vlad-diachenko force-pushed the added_tenant_id_tag_to_tracing_span branch from 62a9c87 to acef697 Compare May 17, 2021 11:31

simonswine reviewed May 17, 2021

View reviewed changes

#4147

f3c5648

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 <vlad.diachenko@grafana.com>

replay reviewed May 19, 2021

View reviewed changes

pkg/querier/tenantfederation/merge_queryable.go Outdated Show resolved Hide resolved

replay reviewed May 19, 2021

View reviewed changes

pkg/util/spanlogger/spanlogger.go Outdated Show resolved Hide resolved

replay reviewed May 19, 2021

View reviewed changes

pkg/util/spanlogger/spanlogger.go Outdated Show resolved Hide resolved

replay approved these changes May 19, 2021

View reviewed changes

vlad-diachenko added 2 commits May 20, 2021 10:21

#4147

4865018

1. fixed tests 2. fixed name of the spans in mergeQuerier Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

#4147 updated CHANGELOG.md

3ff0b80

Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

gouthamve reviewed May 20, 2021

View reviewed changes

pkg/querier/tenantfederation/merge_queryable.go Show resolved Hide resolved

gouthamve approved these changes May 20, 2021

View reviewed changes

pkg/querier/tenantfederation/merge_queryable.go Show resolved Hide resolved

pkg/util/spanlogger/spanlogger.go Show resolved Hide resolved

#4147 passed method's span context to mergeDistinctStringSlice functi…

1d15a71

…on to create inner spans under this context Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

simonswine approved these changes May 20, 2021

View reviewed changes

#4147 added mocktracer package for testing purpose

669b8d2

Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

vlad-diachenko force-pushed the added_tenant_id_tag_to_tracing_span branch from 54b81ba to 669b8d2 Compare May 20, 2021 10:31

pracucci approved these changes May 20, 2021

View reviewed changes

#4147 fixed code style and removed redundant context param that is no…

f0fb3bd

…t used in callback function Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

pracucci reviewed May 20, 2021

View reviewed changes

pkg/querier/tenantfederation/merge_queryable.go Outdated Show resolved Hide resolved

#4147 fixed code style

aed8dab

Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

pracucci enabled auto-merge (squash) May 20, 2021 18:53

pracucci merged commit f675736 into cortexproject:master May 20, 2021

pstibrany reviewed May 20, 2021

View reviewed changes

pkg/util/spanlogger/spanlogger.go Show resolved Hide resolved

#4147 added tenant_id tag to tracing spans #4186

#4147 added tenant_id tag to tracing spans #4186

Uh oh!

Conversation

vlad-diachenko commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonswine left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

simonswine May 17, 2021

Choose a reason for hiding this comment

Uh oh!

simonswine May 17, 2021

Choose a reason for hiding this comment

Uh oh!

vlad-diachenko May 17, 2021

Choose a reason for hiding this comment

Uh oh!

vlad-diachenko May 17, 2021

Choose a reason for hiding this comment

Uh oh!

simonswine May 17, 2021

Choose a reason for hiding this comment

Uh oh!

gouthamve May 20, 2021

Choose a reason for hiding this comment

Uh oh!

simonswine May 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

replay left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gouthamve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

simonswine left a comment

Choose a reason for hiding this comment

Uh oh!

simonswine commented May 20, 2021

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

#4147 added `tenant_id` tag to tracing spans #4186

#4147 added `tenant_id` tag to tracing spans #4186

vlad-diachenko commented May 14, 2021 •

edited

Loading

replay left a comment •

edited

Loading