Query path - Add exhaustive search to combine traces split across blocks #489
Conversation
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
tempodb/tempodb.go
Outdated
| if i == 0 { | ||
| continue | ||
| } | ||
| partialTraces[0], err = tempo_util.CombineTraces(partialTraces[0], partialTraces[i]) |
There was a problem hiding this comment.
This will unmarshal/marshal the trace repeatedly for every partial. It would be more efficient use CombineTraceProtos. Probably not significant in practice (low likelihood).
There was a problem hiding this comment.
Yeah, I agree with this. Let's have store.Find return a slice of all objects it finds. Then the querier can marshal each to proto once and combine.
There was a problem hiding this comment.
Created a variadic version of CombineTraces which I think is what we need here: https://github.com/mdisibio/tempo/blob/combinetracesv/pkg/util/trace.go#L17 This preserves all the error handling/logic of the current function, rather that doing it here in the Find method.
I will say it's a little complicated, as it preserves all edge cases and error messages in CombineTraces. Specifically, if all bytes are identical then we don't even attempt to unmarshal, and the error message is different if none of the byte slices were unmarshalled successfully. If we feel good about this method and relax these conditions, we could simplify things.
There was a problem hiding this comment.
I'm going to go ahead with the current function signature in this PR to avoid including too many changes, but I agree that we need this @mdisibio. I'll work on it in a follow up PR.
There was a problem hiding this comment.
Added some thoughts. Shouldn't there be other places with RunJobs? I'm only seeing one place you changed.
The places @mdisibio is currently removing in his PR?
tempodb/tempodb.go
Outdated
| if i == 0 { | ||
| continue | ||
| } | ||
| partialTraces[0], err = tempo_util.CombineTraces(partialTraces[0], partialTraces[i]) |
There was a problem hiding this comment.
Yeah, I agree with this. Let's have store.Find return a slice of all objects it finds. Then the querier can marshal each to proto once and combine.
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
What this PR does:
Add exhaustive search at the querier to go through all blocks and combine traces.
Involves changes
pool.RunJobs()to not stop after the first hitWhich issue(s) this PR fixes:
Fixes #407
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]