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

Stabilize MetricProducer, allow custom MetricReaders #5835

Merged
merged 4 commits into from
Sep 27, 2023

Conversation

jack-berg
Copy link
Member

Resolves #5317. Resolves #5011.

Spec PR #3685 marked MetricProducer as stable, and was released in spec version 1.25.0.

The stabilization of MetricProducer was blocking custom MetricReaders, so now custom MetricReaders are supported as well.

@jack-berg jack-berg requested a review from a team as a code owner September 15, 2023 15:39
SdkMeterProvider.builder().registerMetricReader(OpenCensusMetrics.attachTo(reader)).build();
SdkMeterProvider.builder()
.registerMetricReader(reader)
.registerMetricProducer(OpenCensusMetricProducer.create())
Copy link
Member Author

Choose a reason for hiding this comment

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

This demonstrates how you would register a custom metric producer with SdkMeterProvider. Nice and tidy.

All the MetricData produced flow to any registered MetricReaders.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great.

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Files Coverage Δ
...etry/exporter/prometheus/PrometheusHttpServer.java 76.06% <100.00%> (-0.21%) ⬇️
...metry/opencensusshim/OpenCensusMetricProducer.java 100.00% <100.00%> (ø)
...telemetry/sdk/metrics/SdkMeterProviderBuilder.java 100.00% <100.00%> (ø)
...opentelemetry/sdk/metrics/export/MetricReader.java 100.00% <ø> (ø)
...metry/sdk/metrics/export/PeriodicMetricReader.java 85.91% <100.00%> (ø)
.../sdk/metrics/internal/export/RegisteredReader.java 78.94% <ø> (ø)
...try/sdk/testing/exporter/InMemoryMetricReader.java 100.00% <100.00%> (ø)
...try/sdk/metrics/export/CollectionRegistration.java 66.66% <66.66%> (ø)
...io/opentelemetry/sdk/metrics/SdkMeterProvider.java 97.64% <90.00%> (-2.36%) ⬇️

... and 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@@ -60,7 +59,7 @@ public final class PrometheusHttpServer implements MetricReader {

private final HttpServer server;
private final ExecutorService executor;
private volatile MetricProducer metricProducer = MetricProducer.noop();
private volatile CollectionRegistration collectionRegistration = CollectionRegistration.noop();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it currently possible to use the OC bridge with the Prometheus exporter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure - the OC metrics will go to any registered MetricReader. Built in MetricReaders include PrometheusHttpServer, PeriodicMetricReader (to support any push based MetricExporter), and InMemoryMetricReader for testing.

SdkMeterProvider.builder().registerMetricReader(OpenCensusMetrics.attachTo(reader)).build();
SdkMeterProvider.builder()
.registerMetricReader(reader)
.registerMetricProducer(OpenCensusMetricProducer.create())
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great.

@jack-berg
Copy link
Member Author

@jkwatson can you take a look when you have a chance?

@jkwatson
Copy link
Contributor

@jkwatson can you take a look when you have a chance?

API changes seem reasonable to me. 👍🏽

@jack-berg jack-berg merged commit f421ef1 into open-telemetry:main Sep 27, 2023
18 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
4 participants