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.

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

Add a configurable max_export_batch_size to the gRPC metrics exporter #2809

Conversation

overmeulen
Copy link
Contributor

@overmeulen overmeulen commented Jul 6, 2022

Description

Add a configurable max_export_batch_size to the gRPC metrics exporter.
This is to avoid reaching the 4MB gRPC limit when observables generate a lot metrics

Fixes #2710

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Added unit tests to test the split logic

Does This PR Require a Contrib Repo Change?

  • Yes.
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@overmeulen overmeulen requested a review from a team as a code owner July 6, 2022 11:51
@overmeulen overmeulen force-pushed the feature/metrics/otlp-export-max-batch-size branch 3 times, most recently from 5ed1b5c to e3cff99 Compare July 6, 2022 13:26
@overmeulen
Copy link
Contributor Author

Thanks for all the comments, I will fix them.
One thing though, I think I'm not going deep enough when splitting the metrics_data.
I should apply the _max_export_batch_size to the number of data-points rather than the number of metrics.
WDYT ?

@srikanthccv
Copy link
Member

Thanks for all the comments, I will fix them. One thing though, I think I'm not going deep enough when splitting the metrics_data. I should apply the _max_export_batch_size to the number of data-points rather than the number of metrics. WDYT ?

If I remember correctly, there will be one data point for each possible attribute set. So the number of metrics should be fine. In the issue, you noticed from your end what was the underlying issue? Was it number of points being high or total number of metrics exceeding the limit overall?

@overmeulen
Copy link
Contributor Author

overmeulen commented Jul 24, 2022

The number of points being high...

@srikanthccv
Copy link
Member

Then you will probably have to divide the batch based on data points.

@overmeulen overmeulen force-pushed the feature/metrics/otlp-export-max-batch-size branch 2 times, most recently from 860d2a9 to 9891ea1 Compare July 26, 2022 12:35
@overmeulen overmeulen force-pushed the feature/metrics/otlp-export-max-batch-size branch from 9891ea1 to 288a15d Compare July 27, 2022 13:48
@ocelotl
Copy link
Contributor

ocelotl commented Jul 27, 2022

@overmeulen thanks, approving. Please don't force push your commits to your PR branch, it makes it hard to review new changes because the old commits don't show up anymore. Don't worry about how it will look in main when merged, before merging github will squash all commits into one.

@overmeulen
Copy link
Contributor Author

@overmeulen thanks, approving. Please don't force push your commits to your PR branch, it makes it hard to review new changes because the old commits don't show up anymore. Don't worry about how it will look in main when merged, before merging github will squash all commits into one.

Noted for next time !

@overmeulen overmeulen force-pushed the feature/metrics/otlp-export-max-batch-size branch from 288a15d to ad2a723 Compare September 1, 2022 12:18
@overmeulen overmeulen force-pushed the feature/metrics/otlp-export-max-batch-size branch from ad2a723 to a063e45 Compare September 1, 2022 12:48
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Sorry for the late comments, feel free to merge maintainers. It can be addressed in a follow up.

auto-merge was automatically disabled September 8, 2022 16:27

Head branch was pushed to by a user without write access

@srikanthccv srikanthccv enabled auto-merge (squash) September 8, 2022 19:02
auto-merge was automatically disabled September 9, 2022 07:15

Head branch was pushed to by a user without write access

@overmeulen
Copy link
Contributor Author

I don't think the last failure is coming from my PR, I see the same failure on other PRs...

@srikanthccv srikanthccv merged commit 41b9e26 into open-telemetry:main Sep 9, 2022
@srikanthccv
Copy link
Member

Contrib SHA needs to be updated. It will be fixed in #2919

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTLPMetricExporter fails to send more than 4MB of data
5 participants