#4147 added tenant_id tag to tracing spans
#4186
Conversation
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
force-pushed
the
added_tenant_id_tag_to_tracing_span
branch
from
5b465ff to
3a3cb4b
Compare
Signed-off-by: Vladyslav Diachenko <[email protected]>
vlad-diachenko
force-pushed
the
added_tenant_id_tag_to_tracing_span
branch
from
62a9c87 to
acef697
Compare
| // 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. |
There was a problem hiding this comment.
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:
That's how that looks like:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yes, it will help to group spans by tenant_id . thanks, i will implement it
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You could instantiate the queriers (call the callback inside LabelValues, etc.) and the right context will be applied.
pkg/util/spanlogger/spanlogger.go
Outdated
| @@ -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 { | |||
There was a problem hiding this comment.
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 <[email protected]>
1. fixed tests 2. fixed name of the spans in mergeQuerier Signed-off-by: Vladyslav Diachenko <[email protected]>
Signed-off-by: Vladyslav Diachenko <[email protected]>
…gSlice function to create inner spans under this context Signed-off-by: Vladyslav Diachenko <[email protected]>
I think with |
Signed-off-by: Vladyslav Diachenko <[email protected]>
vlad-diachenko
force-pushed
the
added_tenant_id_tag_to_tracing_span
branch
from
54b81ba to
669b8d2
Compare
…am that is not used in callback function Signed-off-by: Vladyslav Diachenko <[email protected]>
Signed-off-by: Vladyslav Diachenko <[email protected]>
What this PR does:
tenant_idsto tracing spans if tenant is defined in context.Which issue(s) this PR fixes:
Fixes #4147
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]