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.

Sign up for GitHub

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

Add MetricReader interface #1888

Merged
merged 26 commits into from
Sep 9, 2021
Merged

Add MetricReader interface #1888

Changes from 21 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
435c422
add MetricReader interface
reyang Aug 26, 2021
f50922d
fix link
reyang Aug 26, 2021
38b61a4
Merge branch 'main' into reyang/metrics-sdk-metricreader
reyang Aug 26, 2021
7623003
add the base exporting metricreader
reyang Aug 26, 2021
b3f0fe0
update diagram
reyang Aug 26, 2021
0c2fa70
clarify reentrancy for OnCollect
reyang Aug 27, 2021
4e003a8
describe how MetricReader is registered to MeterProvider
reyang Aug 27, 2021
d60f4ff
Merge branch 'main' into reyang/metrics-sdk-metricreader
reyang Aug 30, 2021
6716bd7
Update specification/metrics/sdk.md
reyang Aug 30, 2021
c42cbc6
Update specification/metrics/sdk.md
reyang Aug 30, 2021
24379f2
Merge branch 'main' into reyang/metrics-sdk-metricreader
reyang Aug 30, 2021
b1b5b2c
update toc
reyang Aug 30, 2021
1305453
address review comments
reyang Aug 31, 2021
ee5f5b8
fix heading
reyang Aug 31, 2021
104d65a
fix typo
reyang Aug 31, 2021
f6bff7c
Merge branch 'main' into reyang/metrics-sdk-metricreader
reyang Aug 31, 2021
fd9dfc4
add clarification and example for multiple readers
reyang Sep 1, 2021
5029131
update the PR based on discussion during the SIG meeting
reyang Sep 3, 2021
ee04190
cleanup
reyang Sep 3, 2021
b2e2fad
cleanup
reyang Sep 3, 2021
86a2c6d
clarify the wording
reyang Sep 3, 2021
47f74e9
adjust wording for pull exporter
reyang Sep 8, 2021
7977fbf
remove base exporting metric reader, mention that it can be an option…
reyang Sep 8, 2021
756d65c
Merge branch 'main' into reyang/metrics-sdk-metricreader
reyang Sep 8, 2021
68f4fdc
change default exporting interval to 60sec
reyang Sep 8, 2021
afafa2c
Merge branch 'main' into reyang/metrics-sdk-metricreader
jmacd Sep 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 127 additions & 20 deletions specification/metrics/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ Table of Contents
* [MeterProvider](#meterprovider)
* [Attribute Limits](#attribute-limits)
* [MeasurementProcessor](#measurementprocessor)
* [MetricReader](#metricreader)
* [Base exporting MetricReader](#base-exporting-metricreader)
* [Periodic exporting MetricReader](#periodic-exporting-metricreader)
* [MetricExporter](#metricexporter)
* [Push Metric Exporter](#push-metric-exporter)
* [Pull Metric Exporter](#pull-metric-exporter)
Expand All @@ -49,10 +52,10 @@ to create an
instance which is stored on the created `Meter`.

Configuration (i.e., [MeasurementProcessors](#measurementprocessor),
[MetricExporters](#metricexporter) and [`Views`](#view)) MUST be managed solely
by the `MeterProvider` and the SDK MUST provide a way to configure all options
that are implemented by the SDK. This MAY be done at the time of MeterProvider
creation if appropriate.
[MetricExporters](#metricexporter), [MetricReaders](#metricreader) and
[Views](#view)) MUST be managed solely by the `MeterProvider` and the SDK MUST
provide a way to configure all options that are implemented by the SDK. This MAY
be done at the time of MeterProvider creation if appropriate.

The `MeterProvider` MAY provide methods to update the configuration. If
configuration is updated (e.g., adding a `MeasurementProcessor`), the updated
Expand Down Expand Up @@ -185,26 +188,26 @@ meter_provider
"Bar",
instrument_name="Y",
aggregation=HistogramAggregation(buckets=[5.0, 10.0, 25.0, 50.0, 100.0]))
.set_exporter(PrometheusExporter())
.add_metric_reader(BaseExportingMetricReader(PrometheusExporter()))
jmacd marked this conversation as resolved.
Show resolved Hide resolved
```

```python
# all the metrics will be exported using the default configuration
meter_provider.set_exporter(ConsoleExporter())
meter_provider.add_metric_reader(PeriodicExportingMetricReader(ConsoleExporter()))
```

```python
# all the metrics will be exported using the default configuration
meter_provider
.add_view("*") # a wildcard view that matches everything
.set_exporter(ConsoleExporter())
.add_metric_reader(PeriodicExportingMetricReader(ConsoleExporter()))
```

```python
# Counter X will be exported as cumulative sum
meter_provider
.add_view("X", aggregation=SumAggregation(CUMULATIVE))
.set_exporter(ConsoleExporter())
.add_metric_reader(PeriodicExportingMetricReader(ConsoleExporter()))
```

```python
Expand All @@ -213,7 +216,7 @@ meter_provider
meter_provider
.add_view("X", aggregation=SumAggregation(DELTA))
.add_view("*", attribute_keys=["a", "b"])
.set_exporter(ConsoleExporter())
.add_metric_reader(PeriodicExportingMetricReader(ConsoleExporter()))
```

### Aggregation
Expand Down Expand Up @@ -526,6 +529,93 @@ measurements using the equivalent of the following naive algorithm:
return boundaries.length
```

## MetricReader
reyang marked this conversation as resolved.
Show resolved Hide resolved

`MetricReader` is an interface which provides the following capabilities:

* Collecting metrics from the SDK.
* Handling the [ForceFlush](#forceflush) and [Shutdown](#shutdown) signals from
jmacd marked this conversation as resolved.
Show resolved Hide resolved
the SDK.

The SDK MUST support multiple `MetricReader` instances to be registered on the
same `MeterProvider`, and the [MetricReader.Collect](#collect) invocation on one
`MetricReader` instance SHOULD NOT introduce side-effects to other `MetricReader`
instances. For example, if a `MetricReader` instance is receiving metric data
points that have [delta temporality](./datamodel.md#temporality), it is expected
that SDK will update the time range - e.g. from (T<sub>n</sub>, T<sub>n+1</sub>]
to (T<sub>n+1</sub>, T<sub>n+2</sub>] - **ONLY** for this particular
`MetricReader` instance.

```text
+-----------------+ +--------------+
| | Metrics... | |
| In-memory state +------------> MetricReader |
| | | |
+-----------------+ +--------------+

+-----------------+ +--------------+
| | Metrics... | |
| In-memory state +------------> MetricReader |
| | | |
+-----------------+ +--------------+
```

The SDK SHOULD provide a way to allow `MetricReader` to respond to
[MeterProvider.ForceFlush](#forceflush) and [MeterProvider.Shutdown](#shutdown).
Individual language clients can decide the language idiomatic approach, for
example, as `OnForceFlush` and `OnShutdown` callback functions.

### MetricReader operations
jmacd marked this conversation as resolved.
Show resolved Hide resolved

#### Collect

Collects the metrics from the SDK. If there are [asynchronous
Instruments](./api.md#asynchronous-instrument) involved, their callback
functions will be triggered.

`Collect` SHOULD provide a way to let the caller know whether it succeeded,
jsuereth marked this conversation as resolved.
Show resolved Hide resolved
failed or timed out.

`Collect` does not have any required parameters, however, individual language
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
clients MAY choose to add parameters (e.g. callback, filter, timeout).
Individual language clients MAY choose the return value type, or do not return
anything.

### Base exporting MetricReader
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?


This is an implementation of the `MetricReader` which collects metrics when
[Collect](#collect) is called. and passes the metrics to the configured
[MetricExporter](#metricexporter).

**Note:** The name **"Base" exporting MetricReader** has nothing to do with
object oriented programming or class inheritance / hierarchy. In case it might
introduce confusion in certain programming languages, individual language client
CAN choose a different wording.

Configurable parameters:

* `exporter` - the exporter where the metrics are sent to.

If the configured exporter only supports [pull mode](#pull-metric-exporter):

* [Collect](#collect) SHOULD only be called when the [Pull Metric
jsuereth marked this conversation as resolved.
Show resolved Hide resolved
Exporter](#pull-metric-exporter) triggers the scraping, otherwise it SHOULD be
treated as an error.

### Periodic exporting MetricReader

This is an implementation of the `MetricReader` which collects metrics based on
a user-configurable time interval, and passes the metrics to the configured
[Push Metric Exporter](#push-metric-exporter).

Configurable parameters:

* `exporter` - the push exporter where the metrics are sent to.
* `exportIntervalMillis` - the time interval in milliseconds between two
consecutive exports. The default value is 5000 (milliseconds).
jsuereth marked this conversation as resolved.
Show resolved Hide resolved
* `exportTimeoutMillis` - how long the export can run before it is cancelled.
The default value is 30000 (milliseconds).

## MetricExporter

`MetricExporter` defines the interface that protocol-specific exporters MUST
Expand All @@ -540,17 +630,29 @@ The following diagram shows `MetricExporter`'s relationship to other components
in the SDK:

```text
+-----------------+ +-----------------------+
| | Metrics... | |
| In-memory state +------------> MetricExporter (push) +--> Another process
| | | |
+-----------------+ +-----------------------+

+-----------------+ +-----------------------+
| | Metrics... | |
| In-memory state +------------> MetricExporter (pull) +--> Another process (scraper)
| | | |
+-----------------+ +-----------------------+
+-----------------+ +-----------------------------+
| | Metrics... | |
| In-memory state +------------> Base exporting MetricReader |
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jsuereth jsuereth Sep 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

| | | |
+-----------------+ | +-----------------------+ |
| | | |
| | MetricExporter (push) +-----> Another process
reyang marked this conversation as resolved.
Show resolved Hide resolved
| | | |
| +-----------------------+ |
| |
+-----------------------------+

+-----------------+ +-----------------------------+
| | Metrics... | |
| In-memory state +------------> Base exporting MetricReader |
jmacd marked this conversation as resolved.
Show resolved Hide resolved
| | | |
+-----------------+ | +-----------------------+ |
| | | |
| | MetricExporter (pull) +-----> Another process (scraper)
reyang marked this conversation as resolved.
Show resolved Hide resolved
| | | |
| +-----------------------+ |
| |
+-----------------------------+
```

Metric Exporter has access to the [pre-aggregated metrics
Expand Down Expand Up @@ -650,6 +752,11 @@ Pull Metric Exporter reacts to the metrics scrapers and reports the data
passively. This pattern has been widely adopted by
[Prometheus](https://prometheus.io/).

Implementors MAY choose the best idiomatic design for their language. For
jmacd marked this conversation as resolved.
Show resolved Hide resolved
example, they could apply the [Push Metric Exporter
interface](#push-metric-exporter) design for consistency, or they could design a
completely different pull exporter interface.

## Defaults and Configuration

The SDK MUST provide the following configuration parameters for Exemplar
Expand Down