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 config to enable Default Exponential Histogram for Prometheus Exporter #6541

Conversation

Abhishekkr3003
Copy link
Contributor

This PR adds support for config otel.exporter.prometheus.metrics.default.histogram.aggregation by which we can make BASE2_EXPONENTIAL_BUCKET_HISTOGRAM as the default histogram aggregation for Prometheus exporter.

@Abhishekkr3003 Abhishekkr3003 requested a review from a team June 27, 2024 11:20
Copy link

codecov bot commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.31%. Comparing base (38aefff) to head (9cba997).
Report is 16 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6541      +/-   ##
============================================
- Coverage     90.59%   90.31%   -0.28%     
- Complexity     6255     6265      +10     
============================================
  Files           689      690       +1     
  Lines         18698    18776      +78     
  Branches       1843     1854      +11     
============================================
+ Hits          16939    16958      +19     
- Misses         1201     1258      +57     
- Partials        558      560       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Abhishekkr3003 Abhishekkr3003 force-pushed the prometheus-exponential-histogram-config branch from a37105a to 46fdadc Compare June 27, 2024 11:43
@jkwatson
Copy link
Contributor

Hello, @Abhishekkr3003 . The environment variable/configuration option you're introducing here doesn't appear to be one of the official configuration options (see here for the full list of official env vars): https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md

In addition, I believe there is a moratorium currently on introducing new environment variables until the configuration specification is complete.

If we are going to introduce a new environment variable here, we would absolutely have to make it an "experimental" env var, and have the word "experimental" in the name of the config option. See here for an example: https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/common/src/main/java/io/opentelemetry/exporter/internal/ExporterBuilderUtil.java#L44

However, I would strongly recommend that you bring this up at the next Java SIG meeting to see if there is an appetite to add new configuration options like this.

* the protobuf java bindings, and assert against the string representation.
*/
@Test
void histogramDefaultBase2ExponentialHistogram() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

@Abhishekkr3003 I added a test which confirms that base2 exponential histograms are in fact configured by requesting the protobuf binary format from the prometheus server, parsing it to equivalent java bindings, and asserting against its string represetnation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @jack-berg , for continuing it, I have not been getting time to contribute here lately.

@jack-berg jack-berg merged commit e2936d4 into open-telemetry:main Aug 8, 2024
15 checks passed
breedx-splk pushed a commit to breedx-splk/opentelemetry-java that referenced this pull request Aug 12, 2024
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.

3 participants