Fix batch processor metrics reorder, improve performance #3034
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bogdandrutu
force-pushed
the
fixmetricsreorder
branch
from
1f19db8
to
18bf0e7
Compare
jrcamp
approved these changes
Apr 27, 2021
|
|
||
| // If possible to move all metrics do that. | ||
| srcMetricsLen := srcIm.Metrics().Len() | ||
| if size-totalCopiedMetrics >= srcMetricsLen { |
There was a problem hiding this comment.
This might read a little cleaner:
Suggested change
| if size-totalCopiedMetrics >= srcMetricsLen { | |
| remainingSpace := size - totalCopiedMetrics | |
| if srcMetricsLen <= remainingSpace { | |
| totalCopiedMetrics += srcMetricsLen |
| srcIm.Metrics().RemoveIf(func(srcMetric pdata.Metric) bool { | ||
| // If we are done skip everything else. | ||
| if totalCopiedMetrics == size { | ||
| return false |
There was a problem hiding this comment.
Nit: it would be nicer if RemoveIf had a way to break the iteration. In this case we will continue to iterate but there is no need, it is pointless. (BTW, RemoveAt would allow this :-) )
There was a problem hiding this comment.
I know, was thinking to actually return a slice of pdata to benefit of the fact that we iterate over all anyway, but I like your suggestion about returning an enum. Will do a quick benchmark to confirm the switch does not affect too much.
| srcRm.InstrumentationLibraryMetrics().RemoveIf(func(srcIm pdata.InstrumentationLibraryMetrics) bool { | ||
| // If we are done skip everything else. | ||
| if totalCopiedMetrics == size { | ||
| return false |
There was a problem hiding this comment.
The return true and return false operations for RemoveIf callbacks here are a bit difficult to understand (return false is not self-explanatory). Please add comments to explain what is happening.
A nicer approach perhaps would be to change RemoveIf's callback to return an enum instead of bool. In that case we could write e.g. return pdata.Remove, return pdata.Keep or return pdata.BreakIteration or similar.
There was a problem hiding this comment.
I like it, the "problem" is that we will avoid calling the callback but we still need to iterate over all to shift them to the left in case something was removed.
Benchmarks Before: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkSplitMetrics BenchmarkSplitMetrics-16 6660 170653 ns/op 182889 B/op 3309 allocs/op PASS Process finished with the exit code 0 ``` Benchmarks After: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkSplitMetrics BenchmarkSplitMetrics-16 7858 134259 ns/op 141881 B/op 2596 allocs/op PASS Process finished with the exit code 0 ``` Benchmarks Reference Clone: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkCloneMetrics BenchmarkCloneMetrics-16 8726 127948 ns/op 137816 B/op 2503 allocs/op PASS Process finished with the exit code 0 ``` Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu
force-pushed
the
fixmetricsreorder
branch
from
18bf0e7
to
df1710d
Compare
dashpole
pushed a commit
to dashpole/opentelemetry-collector
that referenced
this pull request
Jun 14, 2021
…try#3034) Benchmarks Before: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkSplitMetrics BenchmarkSplitMetrics-16 6660 170653 ns/op 182889 B/op 3309 allocs/op PASS Process finished with the exit code 0 ``` Benchmarks After: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkSplitMetrics BenchmarkSplitMetrics-16 7858 134259 ns/op 141881 B/op 2596 allocs/op PASS Process finished with the exit code 0 ``` Benchmarks Reference Clone: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkCloneMetrics BenchmarkCloneMetrics-16 8726 127948 ns/op 137816 B/op 2503 allocs/op PASS Process finished with the exit code 0 ``` Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
ankitnayan
pushed a commit
to SigNoz/opentelemetry-collector
that referenced
this pull request
Jul 27, 2021
…try#3034) Benchmarks Before: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkSplitMetrics BenchmarkSplitMetrics-16 6660 170653 ns/op 182889 B/op 3309 allocs/op PASS Process finished with the exit code 0 ``` Benchmarks After: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkSplitMetrics BenchmarkSplitMetrics-16 7858 134259 ns/op 141881 B/op 2596 allocs/op PASS Process finished with the exit code 0 ``` Benchmarks Reference Clone: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/processor/batchprocessor cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkCloneMetrics BenchmarkCloneMetrics-16 8726 127948 ns/op 137816 B/op 2503 allocs/op PASS Process finished with the exit code 0 ``` Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Benchmarks Before:
Benchmarks After:
Benchmarks Reference Clone:
Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com