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

Define MetricProducer as a third-party provider of metric data to MetricReaders #2951

Merged
merged 13 commits into from
Nov 22, 2022

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Nov 15, 2022

Supersedes #2838 and #2722.

Issue: #1175

This is needed to support an OpenCensus metric bridge, which is will be proposed separately. It could also be used to support other bridges as well, such as a Prometheus bridge.

Changes

Define the MetricProducer interface, with one method: Produce(). MetricProducers SHOULD have a single InstrumentationScope, and can be configured with a Resource.

Add a RegisterProducer(metricProducer) function to MetricReader.

MetricProducers SHOULD allow configuration of temporality, but this isn't enforced via the metric reader. This is because the reader's temporality is configured as a function of OpenTelemetry instrument.

Changes from the previous iteration

  • Integrate with the MetricReader instead of MeterProvider (Feedback from Add MetricProducer as a source of external metric data #2838 (comment))
  • This no longer attempts to prevent duplicate Instrumentation Scopes, or limits a bridge to a single scope (it is still recommended to use a single scope).
  • It now recommends that bridges allow configuration of the output temporality.

Things that are left up to implementations

  • How the MetricReader handles multiple metric batches is NOT specified. SDKs are free to pass each metric batch to the exporter individually, or it may attempt to merge batches if, for example, the resource was the same between the batches. This also implies that any merge logic (e.g. handling duplicate instrumentation scopes) is also not specified, and is left up to the implementation.

@jsuereth @bogdandrutu @jmacd @reyang @pirgeo @MadVikingGod @MrAlias @jack-berg

@dashpole dashpole requested review from a team November 15, 2022 16:53
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Overall, I'm supportive - this is a critical part which makes it possible to converge OpenCensus.
I've left several non-blocking comments considering the added part is Experimental.

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Non blocking comments!

specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
@dashpole
Copy link
Contributor Author

I've left several non-blocking comments considering the added part is Experimental.

Question i'm not entire sure of: I've marked the MetricBridge as experimental, but the Register() function is part of the MetricReader, which is stable. Does that make sense? Or would both essentially need to be stable for any stable languages to implement this?

@dashpole dashpole force-pushed the metric_reader_bridge branch 2 times, most recently from 2ed056b to 85b3491 Compare November 17, 2022 19:35
@arminru arminru added spec:metrics Related to the specification/metrics directory area:sdk Related to the SDK labels Nov 21, 2022
@reyang
Copy link
Member

reyang commented Nov 21, 2022

I've left several non-blocking comments considering the added part is Experimental.

Question i'm not entire sure of: I've marked the MetricBridge as experimental, but the Register() function is part of the MetricReader, which is stable. Does that make sense? Or would both essentially need to be stable for any stable languages to implement this?

I feel it is clear to me. Maybe adding a note in the MetricReader.Register() section saying it is Experimental?

specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor Author

I feel it is clear to me. Maybe adding a note in the MetricReader.Register() section saying it is Experimental?

Done.

@dashpole dashpole changed the title Define MetricBridge as a third-party provider of metric data to MetricReaders Define MetricProducer as a third-party provider of metric data to MetricReaders Nov 22, 2022
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM assuming the changelog will be updated.

@dashpole
Copy link
Contributor Author

LGTM assuming the changelog will be updated.

Added a changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants