-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[chore][pkg/stanza] Fixed broken benchmarks #43190
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
Conversation
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.
Great stuff, thanks @gnak-yar! Please take a look at my note for the SeverityMapping benchmark input size.
require.NoError(b, rcv.Shutdown(b.Context())) | ||
} | ||
|
||
func BenchmarkReadLine(b *testing.B) { |
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 agree with removing this benchmark, we have other benchmarks on various levels (File consumer, File input, File Log receiver) and without this quirky setup with b.N
tied to input size.
Co-authored-by: Andrzej Stencel <[email protected]>
Thank you for your contribution @gnak-yar! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help. |
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This is another fix of open-telemetry#43044. Probably one more PR will be needed! Addressed 2 of the benchmark failures in `pkg/stanza/adapter` here. ``` --- FAIL: BenchmarkReadLine receiver_test.go:285: Error Trace: /Users/ray.kang/github.com/gnak-yar/opentelemetry-collector-contrib/pkg/stanza/adapter/receiver_test.go:285 Error: Received unexpected error: '' unsupported type 'file_input' Test: BenchmarkReadLine --- FAIL: BenchmarkParseAndMap receiver_test.go:369: Error Trace: /Users/ray.kang/github.com/gnak-yar/opentelemetry-collector-contrib/pkg/stanza/adapter/receiver_test.go:369 Error: Received unexpected error: unsupported type 'file_input' Test: BenchmarkParseAndMap ``` **BenchmarkReadLine** The `file_input` operator should be registered to a global operator registry before run. The registration is done when the file input operator package is imported, but the package isn't imported so the error occurs. I've removed the benchmark instead of fixing it. Here's the reason. - As far as I understand, this benchmark is to see the `file_input` operator's performance, but we have other benchmarks doing almost the same thing. - [file.BenchmarkReadExistingLogs](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/7e4bf11279e5b454980626237ccd1417b7e0e26a/pkg/stanza/operator/input/file/benchmark_test.go#L23) - [fileconsumer.BenchmarkFileInput](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/7e4bf11279e5b454980626237ccd1417b7e0e26a/pkg/stanza/fileconsumer/benchmark_test.go#L31) - Even the test seems a bit weird to me because the `b.N` loop(`for i := 0; i < b.N; i++ {`) is used for log generation, not for a function to be measured. **BenchmarkParseAndMap** Two reasons of the failure - The `file_input` package is not imported as well. - Unmarshaling yaml configs(line 369 before change) does not work well, as the configs are not supposed to be loaded from yaml directly. For example, the following structure is a part of `fileconsumer.Config`. `yaml.Unmarshaler` does not understand the embeded fields by default(Probably a more tag like `yaml:",inline"` is required). ```go type Config struct { matcher.Criteria `mapstructure:",squash"` attrs.Resolver `mapstructure:",squash"` ... } ``` As far as I understand, `BenchmarkParseAndMap` is to see the performance of `regex_parser`'s severity mappings. So, I've moved the benchmark to `regex_parser` and made it focus on the operator's performance. I'm not familiar with the full history and background. Please let me know if you have any concerns or suggestions! <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#43044 <!--Describe what testing was performed and which tests were added.--> #### Testing ``` cd pkg/stanza/adapter go test -bench='^Benchmark(ReadLine|ParseAndMap)$' # For testing a benchmark moved to the regex parser. cd pkg/stanza/operator/parser/regex go test -bench='^BenchmarkProcessBatch' ``` --------- Co-authored-by: Andrzej Stencel <[email protected]>
Description
This is another fix of #43044. Probably one more PR will be needed!
Addressed 2 of the benchmark failures in
pkg/stanza/adapter
here.BenchmarkReadLine
The
file_input
operator should be registered to a global operator registry before run. The registration is done when the file input operator package is imported, but the package isn't imported so the error occurs.I've removed the benchmark instead of fixing it. Here's the reason.
file_input
operator's performance, but we have other benchmarks doing almost the same thing.b.N
loop(for i := 0; i < b.N; i++ {
) is used for log generation, not for a function to be measured.BenchmarkParseAndMap
Two reasons of the failure
file_input
package is not imported as well.fileconsumer.Config
.yaml.Unmarshaler
does not understand the embeded fields by default(Probably a more tag likeyaml:",inline"
is required).As far as I understand,
BenchmarkParseAndMap
is to see the performance ofregex_parser
's severity mappings. So, I've moved the benchmark toregex_parser
and made it focus on the operator's performance.I'm not familiar with the full history and background. Please let me know if you have any concerns or suggestions!
Link to tracking issue
Fixes
#43044
Testing