TraceQL: Nested set intrinsics - #3497
Conversation
mdisibio
left a comment
mdisibio
left a comment
There was a problem hiding this comment.
Agree, there are some really promising queries that can answered by accessing these columns directly. Looks very straight-forward and 99% LGTM. Can you take a look at couple q's? Probably ok but want to check.
| continue | ||
|
|
||
| case traceql.IntrinsicNestedSetLeft: | ||
| nestedSetLeftExplicit = true |
There was a problem hiding this comment.
Can you check the behavior in this area, and see if these preds work ok with the nils added in the cases above? I'm thinking about a query that invokes both e.g. { nestedSetLeft = 1 } >> { } | select(nestedSetRight).
There was a problem hiding this comment.
this works b/c the columns are stored in the columnPredicates and columnSelectAs maps.
if the explicit intrinsics hit first then they are added to the map and then the logic in selectColumnIfNotAlready prevents them from being overwritten with an OpNone condition.
if the structural intrinsics hit first then they are overwritten when the explicit intrinsics come along.
There was a problem hiding this comment.
i did note and fix an issue in this area. if the structural intrinsic were to be added after the nested set intrinsic then it would have seen a non-empty predicate and not added the nil predicate. added to test for nil predicate
| }, | ||
| // fun way to get the root span | ||
| { | ||
| req: &tempopb.SearchRequest{Query: "{ nestedSetParent = -1 } | select(name)"}, |
There was a problem hiding this comment.
Can you add a case for when we dual purpose read these columns. Similar to the other comment a query like: {nestedSetParent = -1} >> {} | select(name)
There was a problem hiding this comment.
so there is kind of a bug here, but it exists for all spanset operators. for instance this:
{ span.foo = "bar" } >> {}
Will return the value of the attribute "foo" on every matched span even though the condition is on the LHS of the operator. I'm not sure if this is a bug or not.
There was a problem hiding this comment.
Agree it sounds like a bug. But pre-existing so ok for this PR, if I understand you correctly.
| } | ||
|
|
||
| if a.Intrinsic != traceql.IntrinsicNone { | ||
| if a.Intrinsic == traceql.IntrinsicNestedSetLeft { |
There was a problem hiding this comment.
Do you know if these extra checks are enough to show up in benchmarks, or would a switch be ok here? Wondering at what point this method needs another overhaul. For instance there is duplicate logic for .foo fallback to check resource-level and then span-level, between here and engine.
There was a problem hiding this comment.
i will check benches but would guess these are not visible. don't mind swapping to a switch either for aesthetic or perf reasons if that's preferred.
are switches faster than a series of ifs?
21f3f62 to
48b159b
Compare
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
48b159b to
104cf8d
Compare
* add nested set Signed-off-by: Joe Elliott <number101010@gmail.com> * only return nested set params if explicitly requested Signed-off-by: Joe Elliott <number101010@gmail.com> * tests, tests, tests ! Signed-off-by: Joe Elliott <number101010@gmail.com> * lint Signed-off-by: Joe Elliott <number101010@gmail.com> * changelog Signed-off-by: Joe Elliott <number101010@gmail.com> * added/fixed tests Signed-off-by: Joe Elliott <number101010@gmail.com> * fix predicate adding Signed-off-by: Joe Elliott <number101010@gmail.com> --------- Signed-off-by: Joe Elliott <number101010@gmail.com>
2 participants
Expose nested set intrinsics into the traceql language. This allows calling applications to request structural details about spans.