Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pkg/telemetryquerylanguage] Support Inequalities in TQL #13320

Merged
merged 54 commits into from
Sep 2, 2022

Conversation

kentquirk
Copy link
Member

@kentquirk kentquirk commented Aug 13, 2022

Adds inequality support to TQL.

Previously, TQL supported only == and !=, and only between identical types. With this PR, TQL now also supports <, <=, >=, >, and can compare across different numeric types (two objects of the same type are compared as that type, but objects of differing types are promoted to the most general type -- int/float is compared as float, while int32/int64 is compared as int64.

Link to tracking Issue:
Closes #12491.

Testing:

The testing is fairly extensive:

  • The parser has a long set of go/no-go parser tests, just to make sure that the parser can handle various expressions. It is easy to add more tests.
  • The compare function has a large set of tests with all of the types represented on both sides of comparisons. It does not exhaustively check every possible combination, because the expected values are constructed by hand, but it comes fairly close.
  • Because there were some concerns expressed about performance, I've added some benchmarks for the compare functions. On a 2019 intel macbook, an equivalent to the original comparison function (comparing any values for equality) took about 7 ns/op. The replacement takes between 2 and 12 ns/op, depending on the underlying type (nils are fast, strings are slow, numeric values in the middle). There are no allocations.
  • There are more parser tests exercising a wide range of comparison cases.

Documentation:

  • Update TQL documentation to explain comparison rules
  • Update changelog

Benchmark results (the last line is equivalent to the original function):

goos: darwin
goarch: amd64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/pkg/telemetryquerylanguage/tql
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkCompareEQInt-16         	170905822	         7.802 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareEQFloat-16       	149245143	         7.850 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareEQString-16      	120490750	         9.471 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareEQPString-16     	100000000	        10.06 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareEQBytes-16       	100000000	        10.57 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareEQNil-16         	442970434	         2.697 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareNEInt-16         	170016027	         6.994 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareNEFloat-16       	166002114	         7.190 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareNEPFloat-16      	167566688	         7.136 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareNEString-16      	123459964	        10.00 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareNENil-16         	339125064	         3.066 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareLTInt-16         	150001532	         7.734 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareLTPInt-16        	149175864	         8.466 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareLTFloat-16       	164382165	         7.826 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareLTString-16      	100000000	        10.47 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareLTNil-16         	484894050	         2.527 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareLTDiff-16        	172559710	         6.175 ns/op	       0 B/op	       0 allocs/op
BenchmarkCompareEQFunction-16    	183594465	         6.527 ns/op	       0 B/op	       0 allocs/op

@kentquirk

This comment was marked as outdated.

@kentquirk kentquirk marked this pull request as ready for review August 17, 2022 20:28
@kentquirk kentquirk requested a review from a team August 17, 2022 20:28
@kentquirk
Copy link
Member Author

This is failing the impi test because upstream's main branch is currently broken. Once that's fixed I'll rebase, but meanwhile I believe this is ready for review.

@kentquirk
Copy link
Member Author

@TylerHelmuth ^, please

@TylerHelmuth
Copy link
Member

@kentquirk i will start a review. Also, the collector has dropped support for 1.17, so you should be able to use generics from 1.18 now if you want.

@TylerHelmuth
Copy link
Member

@kentquirk the TQL only supports int64 and float64 for numerics, you may be able to reduce some cases in compare.go for types the query language can't interpret natively. Technically Paths could return types like int or float32, but these are incorrect implementations of a Path.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not remove stuff like int32 yet since it's a sunk cost, but I'm willing to. It will reduce both code and test cases if you think we should never need to worry about it.

I think we should probably stick with @bogdandrutu's normal approach of not adding something until we need it. Since the TQL explicitly states it only supports int64 and float64, I think we should drop the other int and float types until the language supports it. Sorry for the extra work!

@kentquirk
Copy link
Member Author

@TylerHelmuth OK, parser now generates a CompareOp instead of strings, and I've removed support for pointers and non-64-bit type variants (and associated tests).

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kentquirk
Copy link
Member Author

@bogdandrutu I'd appreciate your review. This is passing all tests and has been reviewed and approved by @TylerHelmuth.

@jpkrohling
Copy link
Member

@kovrus, would you like to review this one?

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall, I really like the code comments. I noticed two minor things in the documentation.

pkg/telemetryquerylanguage/tql/README.md Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/README.md Outdated Show resolved Hide resolved
@TylerHelmuth
Copy link
Member

@kovrus @evan-bradley please take another look.

@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Aug 31, 2022
@bogdandrutu bogdandrutu merged commit ad061ff into open-telemetry:main Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pkg/telemetryquerylanguage] Add ordering inequalities to comparisons
8 participants