-
Notifications
You must be signed in to change notification settings - Fork 383
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
Consolidate BatchConfig creation and validation via BatchConfigBuilder #1480
Consolidate BatchConfig creation and validation via BatchConfigBuilder #1480
Conversation
Fixing copy paste leftovers
`with_batch_config` function should be used instead.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1480 +/- ##
=======================================
+ Coverage 63.7% 64.2% +0.4%
=======================================
Files 142 142
Lines 19499 19583 +84
=======================================
+ Hits 12437 12584 +147
+ Misses 7062 6999 -63 ☔ View full report in Codecov by Sentry. |
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.
Thanks for the cleanup. There is some code repetition for BatchConfigBuilder in span_processor, and log_processor (not coming from this PR), and it would be beneficial to refactor this as a shared component in a future PR.
Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com>
Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com>
impl Default for BatchConfigBuilder { | ||
/// Create a new [`BatchConfigBuilder`] initialized with default batch config values as per the specs. | ||
/// The values are overriden by environment variables if set. | ||
/// For a list of supported environment variables see [Batch LogRecord Processor](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#batch-logrecord-processor). |
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.
Lets document it in this repo itself. Linking to spec is okay, but if spec adds something new and is not implemented here, it'll lead to confusion.
@CosminLazar Do you plan to tackle this suggestion in this PR? Its totally okay to have it as a separate PR. |
Will give it a look in a new PR |
Some minor things left, better tackled in follow up PRs. merging now. |
Many thanks for your contributions! |
@lalitb Can you give some hints on how that refactor might look like? Except extracting the environment variables read logic and introducing an internal configuration type to hold those values, I have a bit of a hardtime figuring out what a good abstraction might look like here with my rust skills so far :) |
Design discussion issue (if applicable) #1471
Changes
This PR builds on top of #1471 and aims at consolidating the
BatchConfig
creation for both spans and logs via aBatchConfigBuilder
. TheBatchConfigBuilder
will be responsible for:max_export_batch_size
must be less than or equal tomax_queue_size
),This PR introduces breaking changes ❗:
BatchLogProcessorBuilder
andBatchSpanProcessorBuilder
no longer supports fine grainedBatchConfig
configurations, onlywith_batch_config
is leftBatchConfig
is now a simple data holder and no loger offers mutating functionsMerge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes