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

Clarify MetricReader parameters and temporality controls #2404

Merged
merged 28 commits into from
Mar 18, 2022

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Mar 5, 2022

Fixes #2403.
Replaces #2314.
Replaces #2407.
Fixes #2065.
Part of #2072.

Changes

Clarify the role of the MetricReader in determining the default aggregation, temporality, and default_enabled properties. Clarify the Metric Exporter's "associated" MetricReader and that when auto-configuring an exporter, the Metric Reader's defaults MAY be determined in ways specific to the exporter.

The present form of this PR has a single environment variable to control variable temporality of the OTLP exporter. Note that with 5 instruments having temporality and 2 choices, there are 32 possible settings for temporality across the instruments. I would prefer not to have 5 environment variables, because not very many of the configurations are actually expected.

Therefore, this proposal uses a single environment variable with two specified values, CUMULATIVE (the default), DELTA (where Counters, Async Counters and Histograms all use Delta, UpDownCounter and Async UpDownCounter uses Cumulative).

There's a third value I've proposed in #2314, that would be STATELESS, where synchronous instruments choose Delta and asynchronous instruments choose Cumulative. I do not think we should have 5 environment variables, because not many more than 3 out of the possible 32 combinations are actually going to happen. I'm happy to leave STATELESS out of the current proposal because (speaking as a vendor), my needs are met without it. Given that a MetricReader supports configuring the default temporality as a function of instrument kind, we're able to fill in any of the 29 unspecified temporality combinations through a programmatic interface. It means we will be able to set the STATELESS behavior in our launchers even if the environment variable does not recognize that setting.

@jmacd jmacd requested review from a team March 5, 2022 02:21
@jmacd jmacd added spec:metrics Related to the specification/metrics directory release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs area:sdk Related to the SDK labels Mar 5, 2022
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

LGTM, I think this clarifies more, but I have a small question about the need of having "preferred" temporality at all.

specification/metrics/sdk.md Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Mar 7, 2022

I have prepared #2407, which is an alternative to this one. This specifies more, #2407 specifies less.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Just a minor comment, otherwise i think this provides a good path moving forward.

specification/metrics/sdk.md Outdated Show resolved Hide resolved
@alanwest
Copy link
Member

alanwest commented Mar 9, 2022

I'm liking the direction of this PR. I just had a few questions for clarification.

#2407 alone as discussed in today's SIG meeting won't work - that is we need to continue to maintain some way to configure aggregation temporality. even if the intent was for 2407 to be an intermediate step.

@jmacd
Copy link
Contributor Author

jmacd commented Mar 9, 2022

FYI I will work to clarify the questions posed in this PR today.

@jmacd
Copy link
Contributor Author

jmacd commented Mar 10, 2022

@open-telemetry/specs-metrics-approvers I've expanded the scope of this PR following Tuesday's SIG meeting. Mainly, I restored the OTLP exporter environment variable that expresses what was formerly known as a "preference". Here I'm defining this as the "default aggregation temporality policy", because the default aggregation temporality is a MetricReader setting that is made "on the basis of instrument kind".

In the latest round of editing, I tried to strengthen our understanding of MetricReader. It's not an interface the way I understand it, it's more like an abstract base class for the thing that registers exporters and provides their default settings. Please take a look.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

This PR definitely clarifies who is responsible of doing what. I like that MetricExporter does not have a "preferred temporality API" (is this true?) anymore.

specification/metrics/sdk_exporters/otlp.md Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Mar 16, 2022

@bogdandrutu and @cijothomas please see if efb040f helps.

I like that MetricExporter does not have a "preferred temporality API" (is this true?) anymore.

Yes, there's no API like that in the exporter in my understanding. However, when the SDK auto-configures an exporter there may be a need to configure temporality on a case-by-case basis, with the OTLP exporter being a prime example.

@dyladan
Copy link
Member

dyladan commented Mar 17, 2022

@bogdandrutu and @cijothomas please see if efb040f helps.

I like that MetricExporter does not have a "preferred temporality API" (is this true?) anymore.

Yes, there's no API like that in the exporter in my understanding. However, when the SDK auto-configures an exporter there may be a need to configure temporality on a case-by-case basis, with the OTLP exporter being a prime example.

This is the only point I'm still not completely sure on. Why would we make users configure the temporality manually with the metric reader rather than the metric reader getting temporality configuration from the exporter?

To be clear I am not trying to block or delay this PR at all with this question, merely understand the reasoning for that change.

@cijothomas
Copy link
Member

@bogdandrutu and @cijothomas please see if efb040f helps.

I like that MetricExporter does not have a "preferred temporality API" (is this true?) anymore.

Yes, there's no API like that in the exporter in my understanding. However, when the SDK auto-configures an exporter there may be a need to configure temporality on a case-by-case basis, with the OTLP exporter being a prime example.

This is the only point I'm still not completely sure on. Why would we make users configure the temporality manually with the metric reader rather than the metric reader getting temporality configuration from the exporter?

To be clear I am not trying to block or delay this PR at all with this question, merely understand the reasoning for that change.

There is the following wording in the spec "OpenTelemetry language implementations MAY support automatically configuring the MetricReader to use for an Exporter".
Which I interpret as - If an SDK wants to make it easier for users, it can, by allowing Reader to automatically infer the temporality from the Exporter. Spec is not prohibiting you from doing that. Neither it is enforcing to offer that capability.

Also spec is silent on "How to infer the temporality from an Exporter". I think this is intentional, as languages may have some idiomatic way of expressing it.

@jmacd
Copy link
Contributor Author

jmacd commented Mar 17, 2022

@dyladan I like @cijothomas's answer. It does feel natural for an exporter to make available its preferred temporality, but I'm not sure about making it a requirement; auto-configuring exporters would have a degree of consistency that I suspect you're after.

Since we have some exporters where there are more than one reasonable temporality configuration, I feel that forcing the exporter to provide a preference makes more work for exporters, since if the user wants to change the configuration they have to have a way to change every specific exporter's configuration. Since exporters are 1:1 with readers, it seems simpler to specify that readers support this configuration change so that every exporter doesn't have to. Still, I can see adding a mechanism to get the preferred default when auto-configuring exporters. For the OTLP Exporter, it would interpret the environment variable to decide. This sounds like a good design, not sure it has to be specified this way.

@reyang reyang merged commit 99d1404 into open-telemetry:main Mar 18, 2022
@jmacd jmacd deleted the jmacd/view_tempo branch March 18, 2022 15:51
@bogdandrutu
Copy link
Member

@cijothomas

Which I interpret as - If an SDK wants to make it easier for users, it can, by allowing Reader to automatically infer the temporality from the Exporter. Spec is not prohibiting you from doing that. Neither it is enforcing to offer that capability.

Also spec is silent on "How to infer the temporality from an Exporter". I think this is intentional, as languages may have some idiomatic way of expressing it.

I would definitely not do that by having a new concept "preferred" temporality with some values that are not defined by the spec. But that is just me, I would simply expose a otlp.NewReader() a "factory method" to create reader + exporter in that exporter package instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:metrics Related to the specification/metrics directory
Projects
None yet