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 ConfigurableMetricReaderProvider SPI #5755

Merged
merged 2 commits into from
Aug 24, 2023

Conversation

jack-berg
Copy link
Member

The lack of an SPI for providing MetricReaders is a problem for pull based readers like prometheus (actually, prometheus is currently the only pull based reader). This was true back when I distributed all the autoconfigure code to the modules of each respective exporter, but I was able to get around it with a hack that had opentelemetry-exporter-prometheus implement the AutoConfigurationCustomizer, and directly customize the SdkMeterProviderBuilder.

This hack isn't possible with the file based configuration I'm working on because there is currently no ability to customize. I.e. AutoConfigurationCustomizer isn't in the picture. This may change in the future, but as of now file based configuration interprets exactly the context of a configuration file, ignoring any other environment variables or customization.

This PR adds a new ConfigurableMetricReaderProvider SPI to accommodate pull readers. Its currently in an *.internal.* package because technically the metric SDK doesn't officially support custom pull readers.

Regardless of what file configuration ends up doing with respect to supporting customization outside of what's specified in a configuration file, this is a good SPI to have.

@jack-berg jack-berg requested a review from a team August 23, 2023 17:29
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Patch coverage: 93.33% and project coverage change: -0.08% ⚠️

Comparison is base (1b2307b) 91.31% compared to head (fd8c400) 91.24%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5755      +/-   ##
============================================
- Coverage     91.31%   91.24%   -0.08%     
- Complexity     5151     5198      +47     
============================================
  Files           577      582       +5     
  Lines         15287    15413     +126     
  Branches       1447     1480      +33     
============================================
+ Hits          13959    14063     +104     
- Misses          915      930      +15     
- Partials        413      420       +7     
Files Changed Coverage Δ
.../sdk/autoconfigure/MeterProviderConfiguration.java 100.00% <ø> (ø)
...sdk/autoconfigure/MetricExporterConfiguration.java 95.74% <90.00%> (-4.26%) ⬇️
...theus/internal/PrometheusMetricReaderProvider.java 100.00% <100.00%> (ø)

... and 12 files with indirect coverage changes

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

@trask
Copy link
Member

trask commented Aug 23, 2023

technically the metric SDK doesn't officially support custom pull readers

TIL

@jack-berg
Copy link
Member Author

Yeah, see the javadoc.

Once the MetricProducer SDK extension goes stable in the spec I think we'd be able to fully support custom MetricReaders. Its an interesting data point that we haven't really received much feedback that the lack of support is a problem. I suppose most metric exporters are push based 🤷.

@jack-berg jack-berg merged commit e0a0b77 into open-telemetry:main Aug 24, 2023
17 checks passed
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