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

Add MetricReader interface #1888

Merged
merged 26 commits into from
Sep 9, 2021
Merged
Changes from 5 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
144 changes: 133 additions & 11 deletions specification/metrics/sdk.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ Table of Contents
* [MeterProvider](#meterprovider)
* [Attribute Limits](#attribute-limits)
* [MeasurementProcessor](#measurementprocessor)
* [MetricReader](#metricreader)
* [Periodic exporting MetricReader](#periodic-exporting-metricreader)
* [MetricExporter](#metricexporter)
* [Push Metric Exporter](#push-metric-exporter)
* [Pull Metric Exporter](#pull-metric-exporter)
Expand Down Expand Up @@ -411,6 +413,114 @@ active span](../trace/api.md#context-interaction)).
+------------------+
```

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

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

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

### 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. Then the [OnCollect callback](#oncollect) will be
jmacd marked this conversation as resolved.
Show resolved Hide resolved
jmacd marked this conversation as resolved.
Show resolved Hide resolved
called with the collected metrics from the in-memory state.

`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
client MAY choose to support optional parameters (e.g. filter, timeout).

#### OnCollect

Callback function which will be triggered by [Collect](#collect).
reyang marked this conversation as resolved.
Show resolved Hide resolved

`OnCollect` SHOULD provide a way to indicate whether it succeeded, failed or
timed out.

`OnCollect` MUST accept the following parameters:

* A `batch` of metrics. The exact data type of the batch is language specific,
typically it is some kind of list, e.g. it could be an enumerator that travels
through the in-memory state.

Individual language client MAY choose to add optional parameters (e.g. timeout).
reyang marked this conversation as resolved.
Show resolved Hide resolved

#### OnForceFlush

Callback function which will be triggered by [ForceFlush](#forceflush).

`OnForceFlush` SHOULD provide a way to let the caller know whether it succeeded,
failed or timed out.

`OnForceFlush` SHOULD complete or abort within some timeout. `OnForceFlush` can
be implemented as a blocking API or an asynchronous API which notifies the
caller via a callback or an event. OpenTelemetry client authors can decide if
they want to make the flush timeout configurable.

#### OnShutdown

Callback function which will be triggered by [Shutdown](#shutdown). This is an
opportunity for `MetricReader` to perform any required cleanup.

`OnShutdown` should be called only once for each `MetricReader` instance.

`OnShutdown` should not block indefinitely (e.g. if it attempts to flush the
data and the destination is unavailable). OpenTelemetry client authors can
decide if they want to make the shutdown timeout configurable.

### 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
[OnCollect](#oncollect) is called. and passes the metrics to the configured
[MetricExporter](#metricexporter).

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.
* Individual language client MAY decide if [OnForceFlush](#onforceflush) will
reyang marked this conversation as resolved.
Show resolved Hide resolved
return immediately (with an indication of failure) or wait for the next
[OnCollect](#oncollect) call to complete.

### 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 @@ -425,17 +535,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

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