-
Notifications
You must be signed in to change notification settings - Fork 514
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
TraceQL Perf: Reduce metadata retrieved by implementing two pass iteration. #2119
Conversation
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
for _, ss := range filteredSpansets { | ||
for _, s := range ss.Spans { | ||
span := s | ||
i.currentSpans = append(i.currentSpans, span) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand why this is necessary. If the engine contains a by()
then i.filter(*spanset)
may split it up into multiple spansets and that should be preserved and returned, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this is due to the way the metadata portion of the iterators works. this is ok as long as their is only one spanset per trace, but as discussed this will break once we add by()
.
should we resolve this now? or solve it when we write by()
and coalesce()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaning towards solve when adding by()
and coalesce()
so we can benefit from the performance improvements now.
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid work and amazing performance 🚀🚀 99% LGTM, just a few small questions.
Signed-off-by: Joe Elliott <[email protected]>
What this PR does:
Dramatically reduces the amount of data pulled from the backend for "needle in the haystack" queries by implementing a two pass iteration over the data. The first pass retrieves all of the fields necessary to evaluate the query and then metadata is only retrieved for those spans that actually fulfill the query. Previously metadata was pulled along with the span data which resulted in slower queries when there were few matches.
Fixes #2138
e.g. for the query
{ resource.service.name = "foo" }
Previously iterators looked like the following. In this case span metadata is always retrieved.
Now they look like the following. In this case only the data necessary to evaluate the query is pulled, a "filter" callback is called and only those spans that survive the callback pass through the trace and span metadata iterators.
Other changes:
Stringer
to the iterator and predicate interfaces that can be used to generate the dumps above.Future Work:
Benchmarks:
The
BenchmarkBackendBlockTraceQL
was extended to have significantly more test cases. The latest memory improvements caused some of our absolute fastest queries to slow down a bit but it saved huge allocations on the worst queries. I struggle to completely trust the CPU time benchmarks. This bench suite showed a lot of variance on CPU time likely due to thermal throttling on my tiny laptop. Someone should attempt to recreate.Also note these benchmarks do not take full advantage of the change as they don't run the engine filter.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]