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.

Sign up for GitHub

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

Jump to bottom

[TraceQL] Reduce work performed for structural operators due to a bug #3088

Merged
merged 3 commits into from
Oct 31, 2023
Merged

[TraceQL] Reduce work performed for structural operators due to a bug #3088

merged 3 commits into from
Oct 31, 2023

Conversation

joe-elliott
Copy link
Member

@joe-elliott joe-elliott commented Oct 30, 2023 •
edited
Loading

What this PR does:
Prevents additional columns from being added as iterators when doing structural queries. For the query { resource.service.name = "loki-querier" } >> { resource.service.name = "loki-querier" }, the following iterators were generated.

Before:

rebatchIterator: 
	JoinIterator: 0: traceCollector()	
		LeftJoinIterator: 1: batchCollector(true, 0)
		required: 
			LeftJoinIterator: 3: spanCollector(0)
			required: 
				SyncIterator: rs.list.element.ss.list.element.Spans.list.element.Kind : InstrumentedPredicate{2, nil}
			optional: 
				LeftJoinIterator: 4: attributeCollector{}
				required: 
					SyncIterator: rs.list.element.ss.list.element.Spans.list.element.Attrs.list.element.Key : InstrumentedPredicate{0, StringInPredicate{intrinsic(11), }}
				optional: 
					SyncIterator: rs.list.element.ss.list.element.Spans.list.element.Attrs.list.element.Value : InstrumentedPredicate{8, nil}
					SyncIterator: rs.list.element.ss.list.element.Spans.list.element.Attrs.list.element.ValueInt : InstrumentedPredicate{8, nil}
					SyncIterator: rs.list.element.ss.list.element.Spans.list.element.Attrs.list.element.ValueDouble : InstrumentedPredicate{8, nil}
					SyncIterator: rs.list.element.ss.list.element.Spans.list.element.Attrs.list.element.ValueBool : InstrumentedPredicate{8, nil}
				SyncIterator: rs.list.element.ss.list.element.Spans.list.element.NestedSetLeft : InstrumentedPredicate{2, nil}
				SyncIterator: rs.list.element.ss.list.element.Spans.list.element.NestedSetRight : InstrumentedPredicate{2, nil}
		optional: 
			SyncIterator: rs.list.element.Resource.ServiceName : InstrumentedPredicate{0, StringInPredicate{loki-querier, }}
			SyncIterator: rs.list.element.Resource.ServiceName : InstrumentedPredicate{0, StringInPredicate{loki-querier, }}
		SyncIterator: StartTimeUnixNano : InstrumentedPredicate{0, IntBetweenPredicate{0,1001000000000}}
		SyncIterator: EndTimeUnixNano : InstrumentedPredicate{0, IntBetweenPredicate{1000000000000,9223372036854775807}})

After:

rebatchIterator: 
	JoinIterator: 0: traceCollector()	
		LeftJoinIterator: 1: batchCollector(true, 0)
		required: 
			LeftJoinIterator: 3: spanCollector(0)
			required: 
				SyncIterator: rs.list.element.ss.list.element.Spans.list.element.Kind : InstrumentedPredicate{2, nil}
			optional: 
				SyncIterator: rs.list.element.ss.list.element.Spans.list.element.NestedSetLeft : InstrumentedPredicate{2, nil}
				SyncIterator: rs.list.element.ss.list.element.Spans.list.element.NestedSetRight : InstrumentedPredicate{2, nil}
		optional: 
			SyncIterator: rs.list.element.Resource.ServiceName : InstrumentedPredicate{0, StringInPredicate{loki-querier, }}
			SyncIterator: rs.list.element.Resource.ServiceName : InstrumentedPredicate{0, StringInPredicate{loki-querier, }}
		SyncIterator: StartTimeUnixNano : InstrumentedPredicate{0, IntBetweenPredicate{0,1001000000000}}
		SyncIterator: EndTimeUnixNano : InstrumentedPredicate{0, IntBetweenPredicate{1000000000000,9223372036854775807}})

Also finds a minor performance improvement for situations where a span has a nil intrinsic. This happens regularly for queries like { status = error } where most entries in the StatusCode column are nil.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@joe-elliott
prevent extraneous columns on structural operators
ee81c75 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
prevent unnecessary looping on unmatched intrinsics
66636f4 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Copy link
Member Author

Initial benchmarks. I'm suspicious off the +1/2% and will re-run.

name                                          old time/op    new time/op    delta
BackendBlockTraceQL/spanAttNameNoMatch-8        6.20ms ± 3%    6.21ms ± 1%     ~     (p=0.310 n=5+5)
BackendBlockTraceQL/spanAttIntrinsicMatch-8     42.9ms ± 1%    43.7ms ± 1%   +1.78%  (p=0.008 n=5+5)
BackendBlockTraceQL/statusError-8                122ms ± 1%     125ms ± 2%   +2.77%  (p=0.008 n=5+5)
BackendBlockTraceQL/resourceAttNameNoMatch-8    6.12ms ± 1%    6.23ms ± 2%   +1.90%  (p=0.032 n=5+5)
BackendBlockTraceQL/mixedNameNoMatch-8           407ms ± 2%     487ms ±20%     ~     (p=0.056 n=5+5)
BackendBlockTraceQL/desc-8                       2.09s ± 6%     1.63s ± 6%  -22.12%  (p=0.008 n=5+5)

name                                          old speed      new speed      delta
BackendBlockTraceQL/spanAttNameNoMatch-8       273MB/s ± 3%   273MB/s ± 1%     ~     (p=0.310 n=5+5)
BackendBlockTraceQL/spanAttIntrinsicMatch-8    405MB/s ± 1%   398MB/s ± 1%   -1.75%  (p=0.008 n=5+5)
BackendBlockTraceQL/statusError-8              144MB/s ± 1%   140MB/s ± 2%   -2.69%  (p=0.008 n=5+5)
BackendBlockTraceQL/resourceAttNameNoMatch-8   190MB/s ± 1%   186MB/s ± 2%   -1.85%  (p=0.032 n=5+5)
BackendBlockTraceQL/mixedNameNoMatch-8        12.1MB/s ± 2%  10.3MB/s ±18%     ~     (p=0.056 n=5+5)
BackendBlockTraceQL/desc-8                    3.22MB/s ± 6%  1.35MB/s ± 6%  -57.92%  (p=0.008 n=5+5)

name                                          old MB_io/op   new MB_io/op   delta
BackendBlockTraceQL/spanAttNameNoMatch-8          1.69 ± 0%      1.69 ± 0%     ~     (all equal)
BackendBlockTraceQL/spanAttIntrinsicMatch-8       17.4 ± 0%      17.4 ± 0%     ~     (all equal)
BackendBlockTraceQL/statusError-8                 17.5 ± 0%      17.5 ± 0%     ~     (all equal)
BackendBlockTraceQL/resourceAttNameNoMatch-8      1.16 ± 0%      1.16 ± 0%     ~     (all equal)
BackendBlockTraceQL/mixedNameNoMatch-8            4.91 ± 0%      4.91 ± 0%     ~     (all equal)
BackendBlockTraceQL/desc-8                        6.72 ± 0%      2.20 ± 0%  -67.26%  (p=0.008 n=5+5)

name                                          old alloc/op   new alloc/op   delta
BackendBlockTraceQL/spanAttNameNoMatch-8        2.95MB ± 2%    2.93MB ± 3%     ~     (p=0.310 n=5+5)
BackendBlockTraceQL/spanAttIntrinsicMatch-8     3.22MB ± 5%    3.31MB ± 6%     ~     (p=0.421 n=5+5)
BackendBlockTraceQL/statusError-8               14.1MB ± 2%    13.9MB ± 5%     ~     (p=0.222 n=5+5)
BackendBlockTraceQL/resourceAttNameNoMatch-8    2.95MB ± 3%    2.95MB ± 2%     ~     (p=0.690 n=5+5)
BackendBlockTraceQL/mixedNameNoMatch-8          7.48MB ±17%    6.78MB ±11%     ~     (p=0.310 n=5+5)
BackendBlockTraceQL/desc-8                      57.8MB ±84%   45.6MB ±107%     ~     (p=0.310 n=5+5)

name                                          old allocs/op  new allocs/op  delta
BackendBlockTraceQL/spanAttNameNoMatch-8         44.0k ± 0%     44.0k ± 0%     ~     (p=0.968 n=4+5)
BackendBlockTraceQL/spanAttIntrinsicMatch-8      45.4k ± 0%     45.4k ± 0%     ~     (p=0.587 n=5+5)
BackendBlockTraceQL/statusError-8                 142k ± 0%      142k ± 0%     ~     (p=0.381 n=5+5)
BackendBlockTraceQL/resourceAttNameNoMatch-8     44.1k ± 0%     44.1k ± 0%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/mixedNameNoMatch-8           44.7k ± 0%     44.7k ± 0%     ~     (p=0.452 n=5+5)
BackendBlockTraceQL/desc-8                       1.49M ± 6%     0.67M ±14%  -54.98%  (p=0.008 n=5+5)

@joe-elliott
changelog
2c8f583 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott joe-elliott merged commit b1a28f7 into grafana:main Oct 31, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdisibio mdisibio mdisibio approved these changes

@annanay25 annanay25 Awaiting requested review from annanay25 annanay25 is a code owner

@mapno mapno Awaiting requested review from mapno mapno is a code owner

@kvrhdn kvrhdn Awaiting requested review from kvrhdn kvrhdn is a code owner

@zalegrala zalegrala Awaiting requested review from zalegrala zalegrala is a code owner

@electron0zero electron0zero Awaiting requested review from electron0zero electron0zero is a code owner

@ie-pham ie-pham Awaiting requested review from ie-pham ie-pham is a code owner

@stoewer stoewer Awaiting requested review from stoewer stoewer is a code owner

Assignees
No one assigned
Labels
area/query traceql type/performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joe-elliott @mdisibio