Add MetricReader interface

[reyang]

Follow up #1864.

Changes

• Added the MetricReader interface (refer to Add more details to SDK MeterProvider and View #1730 (comment) for the initial discussion)

[jkwatson]

I would personally find it very helpful if we included a sequence diagram for both the pull & push cases in this documentation of how this should work. I'm having trouble wrapping my head around it just looking at the words here (I'm a visual learner). If we had sequence diagrams, I would have a much better idea of what is being presented here.

[reyang]

I would personally find it very helpful if we included a sequence diagram for both the pull & push cases in this documentation of how this should work. I'm having trouble wrapping my head around it just looking at the words here (I'm a visual learner). If we had sequence diagrams, I would have a much better idea of what is being presented here.

I've added the sequence diagrams, got a gut feeling that this PR is becoming too big 🧙‍♂️. Let's discuss more during the upcoming SIG meeting.

[jsuereth]

Nit: Can this example show with PeriodicMetricReader?

I can't think of an example where you'd use just Base for this case over an explicit instance.

[reyang]

For example, I have a Console Exporter, and I don't want it to export data periodically, what I want is "only prints data to the stdout when I press the ENTER key".
If we force people to use PeriodicMetricReader, it could still work and I guess the workaround is to put the exporter interval to 1 million years.
We might also step back and say "it seems to be an over design" and I'm fine to compromise.

[jsuereth]

For your ConsoleExporter example (and any Push -> Pull adaptation), I'd argue what you want (instead of MetricReader) is to specify a helper/adapter for MetricExporter that stores metrics in-memory (performing additional aggregations every export interval) and when you press enter you get the data from the LAST export period.

I believe this is one of the ways the opentelemetry collecter can export to prometheus (the http endpoint).

[reyang]

I've updated the diagram to use periodic exporting MetricReader.
I've moved the pull exporter examples and diagram to the pull exporter section and clarified that these are just examples so folks can do, but it is not required.

[jsuereth]

We discussed offline that this section isn't necessarily providing anything over just the MetricReader interface.

I do think you might want to consider if the PrometheusExporter should be specified as a MetricReader or MetricExporter, or if you're leaving that up to SDKs. IIUC from our discussion, you're planning to leave that up to SDKs right now.

[reyang]

I've updated the wording for pull exporter (e.g. PrometheusExporter) in this commit.

[jsuereth]

Fundamentally there's a few open questions here that I don't think are addressed in the verbage:

• are you specifying that MetricExporter, when it's a "Pull" exporter MUST be able to call some kind of "go collect things now"? IIUC BaseMetricReader offers that capability to MetricExporter so you could have a Push->Pull adapted MetricExporter and manually call collect. I'm against forcing this on SDKs via the specification. If Push->Pull adaptation must go through interval metric reader, I think we have a much simpler system to maintain.
• Will we require PrometheusExporter to extend the MetricExporter interface in the specification? I assume this will be answered in a follow up PR. I think forcing that will limit the utility of MetricReader for pull-export.
• Is Base exporting MetricReader required for SDKs to implement?

[jsuereth]

Reiley and I discussed offline. I added the most important topics (for reference to everyone) and call out my last bits before approving myself:

1. Remove BaseMetricReader as a concept unless it provides behavior beyond the already specified MetricReader interface.
2. Remove Pull MetricExporter from the specification, or clearly denote it's an optional thing you could do, as MetricReader provides the first-class pull exporter support.

Optional; If we could specify more clearly what "collect" looks like to a Reader so I can leverage that in the OpenCensus <-> OTel Bridge, that'd be ideal, but can be dealt with in follow up.

[reyang]

2. Remove Pull MetricExporter from the specification, or clearly denote it's an optional thing you could do, as MetricReader provides the first-class pull exporter support.

I've adjusted the wording. PTAL 47f74e9.
I do think it is important to keep a section for "Pull Metric Exporter" so other places can link to it (e.g. when we talk about the push vs. pull model in concrete exporter requirement).

[jsuereth]

One blocking comment (the default export interval) but others this looks good to go now.

[cijothomas]

Left a comment (about naming suggestion)

[jkwatson]

Thanks!