TraceQL: make comparison to nil symmetric #4869
Conversation
stoewer
changed the title
stoewer
requested review from
joe-elliott,
mdisibio,
mapno,
yvrhdn,
zalegrala,
electron0zero,
ie-pham,
javiermolinar and
carles-grafana
as code owners
pkg/traceql/ast_execute.go
Outdated
| @@ -250,7 +250,7 @@ func (a Aggregate) evaluate(input []*Spanset) (output []*Spanset, err error) { | |||
| count := 0 | |||
| for _, s := range ss.Spans { | |||
| val, err := a.e.execute(s) | |||
| if err != nil { | |||
| if err != nil || val.Equals(&StaticNil) { | |||
There was a problem hiding this comment.
is this related to symmetric traceql? i'm not opposed. it makes sense to ignore nils here but i wonder if this is part of a larger change to have better nil handling across traceql.
There was a problem hiding this comment.
After making the nil check in isMatchingOperand() symmetric the tests for the query {} | avg(.dne) != 0} failed, because the aggregation over the non existing attribute returned results. The explanation is that parts of the logic relied on the asymmetric nil check in isMatchingOperand()
i wonder if this is part of a larger change to have better nil handling across traceql
Good point. I'll run some more tests and look out for other missing nil checks.
There was a problem hiding this comment.
I've checked the aggregates min, max, and sum and they now all return results when they are applied to non existing attributes like this {} | sum(.dne) != 0}.
Since there is a test for this in searchesThatDontMatch here, I assume that the correct behavior is that all those aggregations don't return results.
On the other hand: looking at the results above the sum(.dne) column is populated with nil and technically nil != 0 is true. Therefore returning results is technically also not wrong. I would just like to confirm the correct behavior here...
There was a problem hiding this comment.
The behavior that I've now implemented for avg, sum, min, and max in this commit is as follows:
- Spans without the attributes are skipped from the aggregate
- Spansets without any any span with the aggregated attribute are removed from the results
There was a problem hiding this comment.
@electron0zero was asking questions about nil handling awhile back and we never answered that Q. I personally like the behavior you've chosen. I think it's the most consistent with the language.
Yes, the nil check in |
| // Operations containing nil | ||
| {".foo != nil", traceql.MustExtractFetchSpansRequestWithMetadata(`{.foo != nil}`)}, | ||
| {"nil != .foo", traceql.MustExtractFetchSpansRequestWithMetadata(`{nil != .foo}`)}, | ||
| {"span.http.status_code != nil", traceql.MustExtractFetchSpansRequestWithMetadata(`{span.http.status_code != nil}`)}, | ||
| {"nil != span.http.status_code", traceql.MustExtractFetchSpansRequestWithMetadata(`{nil != span.http.status_code}`)}, |
There was a problem hiding this comment.
I'll consolidate those test cases with similar test cases from #4864 once the other PR is merged
What this PR does:
Make comparison to
nilsuch as{ nil != .attr }symmetric.Which issue(s) this PR fixes:
Fixes #3989
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]