TraceQL: make comparison to nil symmetric - #4869
Conversation
joe-elliott
left a comment
joe-elliott
left a comment
There was a problem hiding this comment.
changelog?
one question but this looks simple enough to approve. did it really only require a single line in isMatchingOperand?
| 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
* Add test case for symmetric .foo != nil
* Fix make .foo != nil symmetric
* Adjust test for isMatchingOperand()
* Fix false matches for {} | avg(.dne) != 0
* More test cases for symmetric .foo != nil
* More cases for symmetric Static.Equals()
* Test cases for queries with aggregates over non existing attrs
* Fix behavior of aggregates over non existing attrs
2 participants
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]