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

[exporter/prometheusremotewrite] Make maxBatchByteSize configurable #23447

Merged
merged 11 commits into from
Oct 11, 2023
Merged

[exporter/prometheusremotewrite] Make maxBatchByteSize configurable #23447

merged 11 commits into from
Oct 11, 2023

Conversation

yotamloe
Copy link
Contributor

@yotamloe yotamloe commented Jun 19, 2023 •
edited by codeboten

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:

exporters:
  prometheusremotewrite:
    endpoint: "https://my-cortex:7900/api/v1/push"
    max_batch_byte_size: 5000000

Fixes #21911

Testing:
Added MaxBatchByteSize to TestLoadConfig(t *testing.T) in config_test.go

Documentation:
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.

@yotamloe yotamloe requested review from a team and Aneurysm9 as code owners June 19, 2023 09:58
@github-actions github-actions bot requested a review from rapphil June 19, 2023 09:59
@yotamloe yotamloe mentioned this pull request Jun 19, 2023
Closed
JaredTan95
JaredTan95 approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member

@JaredTan95 JaredTan95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yotamloe
Copy link
Contributor Author

@yotamloe thx your contribution, BTW, Is there an official doc describing the recommended maxBatchByteSize value?

@JaredTan95 I guess it depends on the capabilities of the remote storage, I left the default value the same as the old static value.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jul 4, 2023
@JaredTan95 JaredTan95 removed the Stale label Jul 4, 2023
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jul 19, 2023
@JaredTan95
Copy link
Member

PTAL @open-telemetry/collector-contrib-approvers

@JaredTan95 JaredTan95 removed the Stale label Jul 19, 2023
songy23
songy23 approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

rapphil
rapphil reviewed Aug 3, 2023
View reviewed changes
exporter/prometheusremotewriteexporter/README.md Outdated
@@ -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
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @yotamloe

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 17, 2023
@dmitryax dmitryax removed the Stale label Aug 17, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Sep 5, 2023
@fatsheep9146 fatsheep9146 removed the Stale label Sep 5, 2023
@fatsheep9146
Copy link
Contributor

ping @yotamloe

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 1, 2023
@fatsheep9146 fatsheep9146 added ready to merge Code review completed; ready to merge by maintainers and removed Stale labels Oct 1, 2023
This was referenced Oct 3, 2023
Closed
Open
@github-actions github-actions bot mentioned this pull request Oct 10, 2023
Closed
@codeboten codeboten merged commit 20b69de into open-telemetry:main Oct 11, 2023
83 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 11, 2023
JaredTan95 pushed a commit to openinsight-proj/opentelemetry-collector-contrib that referenced this pull request Oct 18, 2023
@yotamloe @JaredTan95
[exporter/prometheusremotewrite] Make maxBatchByteSize configurable (o…
f28934a 
…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 <aboten@lightstep.com>
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request Nov 12, 2023
@yotamloe @jmsnll
[exporter/prometheusremotewrite] Make maxBatchByteSize configurable (o…
2217825 
…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 <aboten@lightstep.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rapphil rapphil rapphil approved these changes

@codeboten codeboten codeboten approved these changes

@songy23 songy23 songy23 approved these changes

@JaredTan95 JaredTan95 JaredTan95 approved these changes

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

Assignees

@bogdandrutu bogdandrutu

Labels
exporter/prometheusremotewrite ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Milestone
v0.88.0
Development

Successfully merging this pull request may close these issues.

[exporter/prometheusremotewrite] Make maxBatchByteSize configurable
8 participants
@yotamloe @JaredTan95 @fatsheep9146 @codeboten @rapphil @songy23 @bogdandrutu @dmitryax