-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[exporter/prometheusremotewrite] Make maxBatchByteSize configurable #23447
[exporter/prometheusremotewrite] Make maxBatchByteSize configurable #23447
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.
LGTM
@JaredTan95 I guess it depends on the capabilities of the remote storage, I left the default value the same as the old static value. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
PTAL @open-telemetry/collector-contrib-approvers |
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.
LGTM
@@ -61,6 +61,9 @@ The following settings can be optionally configured: | |||
- `enabled` (default = false): If `enabled` is `true`, a `_created` metric is | |||
exported for Summary, Histogram, and Monotonic Sum metric points if | |||
`StartTimeUnixNano` is set. | |||
- `max_batch_byte_size` (default = `3000000` -> `~2.861 mb`): Maximum size of a batch of |
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.
nit: could we use max_batch_size_bytes
instead? reference for similar property in an unrelated place https://prometheus.io/docs/prometheus/latest/feature_flags/#extra-scrape-metrics
scrape_body_size_bytes
Usually the suffix denotes the unit.
another option is to use something like https://pkg.go.dev/github.com/docker/go-units#FromHumanSize
go-units is used in this project, but I think it is not worth it.
Otherwise I think this is a reasonable change and LGTM.
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.
ping @yotamloe
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.
@rapphil Sorry for the delay just refactored to max_batch_size_bytes
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.
Thank you!
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
ping @yotamloe |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
…pen-telemetry#23447) Adding a feature: Making `maxBatchByteSize` a configurable parameter. This would allow users to adjust it based on the capabilities of their specific remote storage, offering more flexibility and potentially improving performance. Example: ```yaml exporters: prometheusremotewrite: endpoint: "https://my-cortex:7900/api/v1/push" max_batch_byte_size: 5000000 ``` Fixes open-telemetry#21911 **Testing:** <Describe what testing was performed and which tests were added.> Added `MaxBatchByteSize` to `TestLoadConfig(t *testing.T)` in `config_test.go` **Documentation:** <Describe the documentation added.> Added to `README.md`: - `max_batch_byte_size` (default = `3000000` -> `~2.861 mb`): Maximum size of a batch of samples to be sent to the remote write endpoint. If the batch size is larger than this value, it will be split into multiple batches. --------- Co-authored-by: Alex Boten <[email protected]>
…pen-telemetry#23447) Adding a feature: Making `maxBatchByteSize` a configurable parameter. This would allow users to adjust it based on the capabilities of their specific remote storage, offering more flexibility and potentially improving performance. Example: ```yaml exporters: prometheusremotewrite: endpoint: "https://my-cortex:7900/api/v1/push" max_batch_byte_size: 5000000 ``` Fixes open-telemetry#21911 **Testing:** <Describe what testing was performed and which tests were added.> Added `MaxBatchByteSize` to `TestLoadConfig(t *testing.T)` in `config_test.go` **Documentation:** <Describe the documentation added.> Added to `README.md`: - `max_batch_byte_size` (default = `3000000` -> `~2.861 mb`): Maximum size of a batch of samples to be sent to the remote write endpoint. If the batch size is larger than this value, it will be split into multiple batches. --------- Co-authored-by: Alex Boten <[email protected]>
Description:
Adding a feature: Making
maxBatchByteSize
a configurable parameter. This would allow users to adjust it based on the capabilities of their specific remote storage, offering more flexibility and potentially improving performance.Example:
Fixes #21911
Testing:
Added
MaxBatchByteSize
toTestLoadConfig(t *testing.T)
inconfig_test.go
Documentation:
Added to
README.md
:max_batch_byte_size
(default =3000000
->~2.861 mb
): Maximum size of a batch ofsamples to be sent to the remote write endpoint. If the batch size is larger
than this value, it will be split into multiple batches.