[bugfix] Add nil support for tags lookup - #5967
Conversation
| // be sped up by searching for traceDuration first. note that we can only set the predicates if all conditions is true. | ||
| // otherwise we just pass the info up to the engine to make a choice | ||
| for _, cond := range conds { | ||
| if cond.Op == traceql.OpNotExists { |
There was a problem hiding this comment.
this is inconsistent with the way other things are handled but i prefer this. for instance in createSpanIterator we interpret span:name = nil to be span:name = "" which seems wrong.
https://github.com/grafana/tempo/blob/main/tempodb/encoding/vparquet5/block_traceql.go#L1910-L1927
honestly I think neither path is correct. { span:name = nil } is simply an invalid query and should not be executed. an intrinsic cannot be nil. only attributes can.
let's get the panic fixed in this PR, but i'd like the change above. it should also simplify quite a bit of the code.
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Original panic |
| // = nil ? | ||
| if isNotExistSearch { | ||
| pred := parquetquery.NewIncludeNilStringEqualPredicate([]byte(cond.Attribute.Name)) | ||
| iters = append(iters, makeNilIter(columnPathSpanAttrKey, pred, "")) // don't select just filter nils |
There was a problem hiding this comment.
Confirming my understanding: this line (and the others like it) prevent the panic by not dropping into the genericConditions for =nil checks on generic attrs. Because the Operands[0].Type lookup is still in createDistinctAttributeCollector, instead we are catching that one layer higher here.
There was a problem hiding this comment.
Yes, the = nil operation breaks expectations by not passing in an operand, but instead only passing an operator of NotExists. This special handling is also in the normal iterator construction as well.
| // tag requested. only used for tag values. if populated then return tag values for this tag, otherwise return tag names. | ||
| tag traceql.Attribute | ||
| // existsTagName is a callback to check if a tag name has already been seen | ||
| existsTagName func(key tagNameKey) bool |
There was a problem hiding this comment.
I like this change of moving the cache one layer up, but is there a performance or correctness difference? It looks like the data by each distinct attr collector doesn't overlap because they are different scopes anyway. So a map here of all data, is the same as smaller maps in the individual collectors.
Update: OK I think I understand this IS a correctness fix. The cache inside each distinct attr collector is too low, it's caching every individual foo and bar, but we are looking for foo && bar. The top-level map is only populated after that. 👍
There was a problem hiding this comment.
Right, so if you were searching for values of span.foo where resource.baz = "bat" then the distrinctAttrCollector would only surface each value of span.foo the first time it was found due to the local map. If this value happened to be eliminated by the higher level resource condition we would never attempt to collect it again.
| for _, cond := range conds { | ||
| if cond.Op == traceql.OpNotExists { | ||
| // trace-level intrinsic can't be nil | ||
| continue |
There was a problem hiding this comment.
Should this be a return instead? Continuing like the condition doesn't exist I think makes ... && trace:id = nil return everything instead of nothing. (Specifically it returns the resourceIter with no extra filtering). We have been chatting that handling of intrinsics in general is not where we want in this PR, so if we want to include this in the follow up that is fine too.
There was a problem hiding this comment.
I think to catch other trace level conditions it needs to be a continue as written. However it's in a weird place b/c it's not asserting a condition and just forcing the engine to evaluate intrinsic = nil. I did some basic testing and this seems to always evaluate to false, but haven't dug deeper.
What it should really do is just return an error b/c this is an invalid condition. We should also include protections higher up to presumably never pass this condition into the fetch layer.
| ) | ||
|
|
||
| for _, cond := range conditions { | ||
| isNotExistSearch := len(cond.Operands) == 0 && cond.Op == traceql.OpNotExists |
There was a problem hiding this comment.
Is it possible to generate an OpNotExists with operands or does it matter? We are checking this in each place, and wondering if we can simplify.
There was a problem hiding this comment.
We can likely simplify. I didn't write the original functionality so would have to dig deeper.
mdisibio
left a comment
mdisibio
left a comment
There was a problem hiding this comment.
Chatted offline and although there are still outstanding changes we want, this is a significant improvement in correctness and fixes the panic, which would be great to get in.
* [bugfix] support attr = nil for tags * add other vparquet versions * added tests * changelog * Accumulate distinct results at a higher level Signed-off-by: Joe Elliott <number101010@gmail.com> * Add logic to wal blocks Signed-off-by: Joe Elliott <number101010@gmail.com> --------- Signed-off-by: Joe Elliott <number101010@gmail.com> Co-authored-by: Joe Elliott <number101010@gmail.com>
3 participants
What this PR does: Add attr = nil support for tags look up. Without this, tags endpoint would panic when
attr = nilis used in the query. This is mostly just applying the logic from block_traceql.goWhich issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]