Add MetricProducer as a source of external metric data

[dashpole]

Part of #1175

Related to #2730

Required for #2732

Supersedes #2722. The numbering scheme for alternatives caused some confusion, and it was requested that I open a new PR.

Changes

Add a MetricProducer interface to the metrics SDK. It is an optional argument when creating a MetricReader meant to support non-SDK sources of metric data.

Metric data from a MetricProducer is not subject to the MeterProviders or MetricReaders configuration for instruments. This means Views on MeterProviders, and default Aggregations and Aggregation Temporalities configured on MetricReaders are not applied to data from MetricProducers.

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

[dashpole]

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

[jmacd]

Thank you @dashpole. I think resetting the review state here could help us improve this feature. I would like us now to also consider the "ALTERNATIVE" proposal known as option number two in #2722.

I re-read this PR and think with small changes we could have the alternative approach (which I now favor). Whereas you wrote:

+-----------------+            +--------------+
|                 | Metrics... |              |
| MeterProvider   +------------> MetricReader |
|                 |            |              |
+---------------^^+            +--------------+
|  MetricProducer |
+-----------------+

The alternative looks like one (or more) MetricProducers in a pipeline between the Reader and the Exporter.

+-----------------+                 +--------------+              +----------------+
|                 | Internal data   |              | Export data  |                | To exporter...
| MeterProvider   +-----------------+ MetricReader +--------------+ MetricProducer +---------------->
|                 |                 |              |              |                |
+-----------------+                 +--------------+              +----------------+

Most of the PR doesn't have to change much. MetricReaders no longer care, what's left is a question about trade-offs.

• In the alternative proposal, MetricProducers have to be configured for each Reader instead of once per SDK: SDKs could perhaps alleviate this with a helper to make adding producers to all readers easy.
• In the alternative proposal, errors are deferred until the first time metrics are read by the exporter: this seems acceptable.

@dashpole what do you think?

[bogdandrutu]

@jmacd you mentioned that

When used with distributed tracing, a resource can be associated 
with the [TracerProvider](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#tracerprovider) 
when the TracerProvider is created. That association cannot be changed later. 
When associated with a TracerProvider, all Spans produced by any Tracer 
from the provider MUST be associated with this Resource.

Analogous to distributed tracing, when used with metrics,
a resource can be associated with a `MeterProvider`.
When associated with a [`MeterProvider`](../metrics/api.md#meterprovider),
all metrics produced by any `Meter` from the provider will be
associated with this `Resource`.

It's that "analogous" MUST we are defending here. It appears the stable Java SDK has a loophole, and now we have wrinkles in the specification because of it. I recommend @dashpole remove all the wrinkle about Resources "if it is possible for those to conflict", leave the specification stating that each producer has an instrumentation scope, and let the Java SDK document its loophole (i.e., it should document that users are required to use the same resource twice or else be out-of-spec).

I don't think the specification says that resource MUST only be associated the providers. Out of all the concepts so far that is true, and the statement you linked tells how that association must be implemented, but I don't see anywhere where it is forbidden to have other sources of telemetry with their own "Resource".

Because of that I still believe the version where MetricProvider implementing the Reader is ok.

[jmacd]

Because of that I still believe the version where MetricProvider implementing the Reader is ok.

Not sure what you're trying to say. I feel that this is a new topic. I quoted part of the Resource SDK specification. Above the part I quoted,

A [Resource](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/overview.md#resources) is an immutable representation of the entity producing telemetry as [Attributes](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/README.md#attribute).

If we allow the Bridge to configure its own Resource, there's a good chance the bridge will appear as a separate Resource meaning a different entity. I believe the under-specified spirit of the Resource SDK Specification is that we should not encourage a bridge implementation that uses different Resource attributes than the OTel MeterProvider it is associated with.

[dashpole]

I think its slightly confusing for a MetricProducer to receive metrics from a MetricReader. Would this:

+-----------------+                 +--------------+            
|                 | Internal data   |              | To exporter... 
| MeterProvider   +-----------------+ MetricReader +------------->
|                 |                 |              |
+-----------------+                 +------|-------+              
                                  +--------+-------+
                                  |                | 
                                  + MetricProducer +
                                  |                |
                                  +----------------+

instead of this:

+-----------------+                 +--------------+              +----------------+
|                 | Internal data   |              | Export data  |                | To exporter...
| MeterProvider   +-----------------+ MetricReader +--------------+ MetricProducer +---------------->
|                 |                 |              |              |                |
+-----------------+                 +--------------+              +----------------+

be a reasonable approach?

[reyang]

+-----------------+                 +--------------+              +----------------+
|                 | Internal data   |              | Export data  |                | To exporter...
| MeterProvider   +-----------------+ MetricReader +--------------+ MetricProducer +---------------->
|                 |                 |              |              |                |
+-----------------+                 +--------------+              +----------------+

I think there is value of having this component and it should be called "MetricProcessor". Calling it "MetricProducer" will be problematic if later we want to introduce MetricProcessor (and we'll struggle with the ordering of these components in the pipeline).

[reyang]

I think its slightly confusing for a MetricProducer to receive metrics from a MetricReader. Would this:

+-----------------+                 +--------------+            
|                 | Internal data   |              | To exporter... 
| MeterProvider   +-----------------+ MetricReader +------------->
|                 |                 |              |
+-----------------+                 +------|-------+              
                                  +--------+-------+
                                  |                | 
                                  + MetricProducer +
                                  |                |
                                  +----------------+

How about this?

+-----------------+                 +--------------+            
|                 | Internal data   |              | To exporter... 
| MeterProvider   +-----------------> MetricReader +------------->
|                 |                 |              |
+-----------------+                 +------^-------+              
                                           |
+-----------------+                        |
|                 | Bridged data           |
| MetricWriter    +------------------------+
|                 |
+-----------------+

And here is my reason:

• MetricProducer sounds like a broad term, it can also mean a combination of "MeterProvider" and "MetricReader".
• MetricWriter makes it clear that this component is intended for some external systems that handle aggregation already, and use this interface to write/feed the pre-aggregated metrics to the reader (which limits the control of several settings, such as aggregation temporality, view, histogram buckets).

[dyladan]

How about this?

+-----------------+                 +--------------+            
|                 | Internal data   |              | To exporter... 
| MeterProvider   +-----------------> MetricReader +------------->
|                 |                 |              |
+-----------------+                 +------^-------+              
                                           |
+-----------------+                        |
|                 | Bridged data           |
| MetricWriter    +------------------------+
|                 |
+-----------------+

And here is my reason:

• MetricProducer sounds like a broad term, it can also mean a combination of "MeterProvider" and "MetricReader".
• MetricWriter makes it clear that this component is intended for some external systems that handle aggregation already, and use this interface to write/feed the pre-aggregated metrics to the reader (which limits the control of several settings, such as aggregation temporality, view, histogram buckets).

As long as we're discussing naming MetricWriter sounds like something that persists data to a disk or something like that. Maybe MetricBridge since that's what it's actually being used for?

[reyang]

As long as we're discussing naming MetricWriter sounds like something that persists data to a disk or something like that. Maybe MetricBridge since that's what it's actually being used for?

+1 👍 (I see the point where "writer" might be confusing, I think "bridge" or "adapter" might be okay)

[dashpole]

Overall, i'd like to settle on Integrating with the MetricReader vs MeterProvider question before we dive into other details. I'm happy to revisit naming, interfaces, or other aspects after that.

I would summarize the tradeoffs as:

Bridge integrates with MeterProvider (:tada:):

• Simpler configuration UX: Passing the bridge to the MeterProvider is all that needs to be done. No configuration is required for/with each metric reader.

Bridge integrates with MetricReader (:rocket:):

• More logically consistent: It is clear that View configuration of the MeterProvider does not apply to Bridges. It isn't possible for us to make View configuration apply to Bridges because it is based on Instrument, rather than Aggregation.
• Less confusion for unsupported Temporalities if using multiple Readers: When using multiple readers, users might be confused if some readers don't support the temporality of metrics from the bridge, while others do. This integration more explicitly links exporters and bridges.

Orthogonal-issues that I don't believe impact the decision above:

• It is possible to share resource regardless of where this integration happens. We will also have to tackle the issue with how java represents resource regardless of which approach we take.
• It is possible to share interfaces with the SDK to simplify implementation regardless of where the integration happens. I don't believe either implementation will be significantly more complex than the other.
• Depending on the interface we choose for the Bridge, is possible to detect duplicate scopes between bridges/SDK on initialization of meters/producers, or at collection time, or not at all. This also isn't impacted by our choice of where we integrate.

Feel free to propose benefits/drawbacks of the approaches that i've missed. I'd like to take the temperature of the room with 🎉 or 🚀 reactions to this comment.

[reyang]

+1 on "Bridge integrates with MetricReader" - I think most users won't directly interact with the bridge, I would imagine that OpenCensus (or Micrometer, whatever) Bridge to be packaged under some distro/family-pack, so folks can use it easily during the migration, without having to understand all the implementation details.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[dyladan]

I think this PR isn't really stale?

[dashpole]

Yeah, I'm still working on it. But I don't have any updates to share right now.

Edit: I can't reopen, but if someone else can, that would be appreciated.

[dyladan]

I just mean it should be reopened. Closed PRs tend to be lost