Define MetricExporter interfaces

[dyladan]

The specification defines both push and pull metric exporters. These interfaces will need to be defined so that exporters can implement them.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricexporter

Pending merge of open-telemetry/opentelemetry-specification#2106 the specification has clarified the requirements of the exporter interface:

• The exporters should receive a list of Metrics
• Metric contains name, unit, description, meter information, etc, and a list of MetricPoints
• MetricPoint contains timestamps, dimensions, value (or buckets), exemplars, etc, of collected metrics

[legendecas]

I don't find a clear interface of the metrics data of the exporter.batch(data) in the spec: open-telemetry/opentelemetry-specification#2073.

Currently, we define the exporter to batch with a series of MetricRecord. i.e. they are a record of (instrument descriptor, attributes, aggregated value). But I do find implementations like java define the metrics data as (instrument descriptor, points) and the points as (attributes, aggregated value). AFAICT, OTLP protocol defines the exporter to batch data with the interface what otel java sdk is defined like: https://github.com/open-telemetry/opentelemetry-proto/blob/59c488bfb8fb6d0458ad6425758b70259ff4a2bd/opentelemetry/proto/metrics/v1/metrics.proto#L60.

[dyladan]

That was also the same thing we did for the existing SDK and we will likely do the same thing again. Modeling the batch interface after the OTLP spec ensures we have all data required for OTLP export which should be everything in the data model so it is a good shortcut to make sure we're not missing anything important.

[legendecas]

@dyladan sorry but I'm not sure what you are suggesting. Do you suggest we should keep exporter.batch(MetricRecord[]) as same as what we have now, or update it with the OTLP model like exporter.batch(MetricData[]) which groups with metric names (metric record with different attributes were grouped together)?

The latter one (grouped form) can reduce the work on the exporter side if they can send the same metric in a compact form, i.e. they don't need to repeat metric descriptors for each attributes set.

[dyladan]

Both are reasonable options. I was suggesting the second one, but the first one would mean we don't have to update all of our metrics exporters. Fortunately we don't have that many yet.

@legendecas unrelated but i'm not sure how else to contact you. are you on CNCF slack? we're trying to plan the metrics SDK and you've been pretty involved in metrics.

[legendecas]

open-telemetry/opentelemetry-specification#2106 It's been clarified on spec. We should go with the exporter.batch(MetricData[]) which groups with metric names and meters (metric points with different attributes were grouped together) pattren.

[dyladan]

That isn't merged yet, but I'll add it to the issue description.

[srikanthccv]

I'd be happy to pick this one up if no one is already working on it.

[dyladan]

@lonewolf3739 there is a PR by @pichlermarc open but he is out for the holiday. I believe he will be back on tuesday.

#2681

edit: oops I thought this was metric reader not metric exporter. Yes you can feel free to work on this. You may want to look at #2681 to make sure work is compatible and not duplicated.

[srikanthccv]

@legendecas Do we want to model pull exporters around MetricReader? We have an exporter interface defined already which is basically same as PushMetricExporter interface def.

[dyladan]

It is my understanding that yes, pull metric exporters will implement the MetricReader interface.

Configuring a push metric exporter (or similar)

provider.addMetricReader(new PeriodicMetricReader(new PushMetricExporter(config))

Configuring a pull metric exporter (or similar)

provider.addMetricReader(new PullMetricExporter(config))

[srikanthccv]

I am wondering if we want to introduce any new interfaces. I think the existing interfaces are sufficient and we can make use of them. The push type exporters can implement the MetricExporter and prometheus style pull metric exporters can MetricReader interface (operations such as forceFlush will be no-op in the exporter implementation).

[legendecas]

I agree that we don't need another new interface to describe the pulling exportation. In #2681 the MetricReader abstract class is intentionally implemented without the dependency on MetricExporter to allow various patterns of exporting.

[dyladan]

I am wondering if we want to introduce any new interfaces. I think the existing interfaces are sufficient and we can make use of them. The push type exporters can implement the MetricExporter and prometheus style pull metric exporters can MetricReader interface (operations such as forceFlush will be no-op in the exporter implementation).

I think you are suggesting that both pull and push exporters would be implemented as MetricReaders directly rather than having a metric reader that takes a push exporter in the constructor? I would rather go the route we have already been going with a metric reader that takes an exporter because if we don't all push exporters would have to reimplement the logic to periodically collect and then export.

[srikanthccv]

No, you misunderstood me. I am suggesting only the pull exporters will implement the metric reader interface. The exporters like OTLP which are push model implement the MetricExporter interface. We may want to rename it PushMetricExporter to make it more clear. So overall I think there is no need to define a new interfaces. Does it make sense?

[dyladan]

Yeah that makes sense. Renaming to PushMetricExporter interface is definitely more clear I think too.

[legendecas]

@dyladan I think it would be better to have a spec note on the naming, as the spec is already suggesting implementing pull metric exporter as a metric reader.