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

Clarification needed on exporter/reader configuration #2854

Closed
MrAlias opened this issue Oct 4, 2022 · 8 comments
Closed

Clarification needed on exporter/reader configuration #2854

MrAlias opened this issue Oct 4, 2022 · 8 comments
Assignees
Labels
area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory triaged-accepted The issue is triaged and accepted by the OTel community, one can proceed with creating a PR proposal

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Oct 4, 2022

This issue is intended to track an issue identified in the 2022-10-04 specification SIG.

Currently the specification states:

To construct a MetricReader when setting up an SDK, the caller
SHOULD provide at least the following:

  • The exporter to use, which is a MetricExporter instance.
  • The default output aggregation (optional), a function of instrument kind. If not configured, the default aggregation SHOULD be used.
  • The default output temporality (optional), a function of instrument kind. If not configured, the Cumulative temporality SHOULD be used.

However, it also states:

Metric Exporters always have an associated MetricReader. The aggregation and temporality properties used by the OpenTelemetry Metric SDK are determined when registering Metric Exporters through their associated MetricReader.

It was pointed out in today's specification SIG meeting that the first statement about configuration of the SDK needs to be interpreted with the context of the second regarding the exporter. Meaning the optional default output aggregation and temporality configuration is only directly configured with a reader if it does not accept an exporter. This is not clear to all readers of the specification and it was suggested we update the language of the specification.

Proposal

  • Link in the first metric reader section the second metric exporter section
  • Update the configuration language of the metric reader section to state the default output aggregation and temporality need to come from the exporter.
@MrAlias MrAlias added the spec:metrics Related to the specification/metrics directory label Oct 4, 2022
@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 4, 2022

cc @jsuereth @jmacd @tsloughter

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 4, 2022

github actions assigned this to @bogdandrutu but @reyang had said in the SIG meeting he would take this on (with low priority).

@reyang
Copy link
Member

reyang commented Oct 4, 2022

Assigned to myself.

@reyang reyang added area:sdk Related to the SDK triaged-accepted The issue is triaged and accepted by the OTel community, one can proceed with creating a PR proposal labels Oct 4, 2022
@jmacd
Copy link
Contributor

jmacd commented Oct 4, 2022

Thank you @MrAlias! Your proposal sounds good to me.

EDIT: I have apparently mis-understood this discussion! @MrAlias explained how I had not understood the discussion on Tuesday, sorry.

@jmacd
Copy link
Contributor

jmacd commented Oct 6, 2022

I believe we are debating an implementation detail. The quoted sentence,

Metric Exporters always have an associated MetricReader. The aggregation and temporality properties used by the OpenTelemetry Metric SDK are determined when registering Metric Exporters through their associated MetricReader.

Is one that I wrote to allow implementations a wide range of strategies. As in the OpenTelemetry Library Guidelines state (see point 4), Exporters should be separated into protocol-dependent parts and otherwise be kept minimal.

The reason I wrote that sentence the way I did, is so that the MetricReader abstraction is able to absorb all of the configuration functionality and complexity in the SDK, i.e., to keep the Exporters minimal. The current OTel-Go metrics SDK, in my opinion, is perfectly compliant with this specification, where the Reader configures the temporality and aggregation as desired for the exporter--the Reader provides both the default behaviors as well as options to reconfigure the parts that are specified as configurable.

In the OTel-Go SIG today we discussed a hypothetical Statsd exporter being configured for the metrics SDK (not that I expect anyone to do this in the year 2022). The Exporter could, in the current OTel-Go design, be misconfigured such that at runtime the Exporter would receive cumulative temporality. This is not a failure mode that I expect to happen, but if it does the Exporter would be within its rights to return an error on Export().

The user who decides to use a default-configured Statsd Exporter would be given a push-based MetricReader configured appropriately from the environment, and since Statsd does not support configurable temporality there would be no question as to the temporality of the associated Reader. The specification does not currently require the Exporter to have an interface for MetricReaders to receive this configuration directly, that is an implementation detail.

As we discussed in the OTel-Go SIG, if the exporters are required to configure these items themselves, it simply leads to every exporter re-implementing options if they need them. OTLP is the first exporter to have configurable temporality, let's say, but maybe another implementation of the OTLP Exporter using hand-generated protobufs will be implemented -- will it need to also re-implement all the associated configuration?

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 12, 2022

This looks like it is a duplicate of #2746

It seems like a longstanding issue.

@jmacd
Copy link
Contributor

jmacd commented Oct 12, 2022

I am willing to help fix the specification to clarify that MetricReaders are responsible for configuration and that how the defaults are determined is an implementation detail. I wrote the sentence in question,

Metric Exporters always have an associated MetricReader. The aggregation and temporality properties used by the OpenTelemetry Metric SDK are determined when registering Metric Exporters through their associated MetricReader.

I want to call attention to the word "associated". IMO it is meaningless to speak of an exporter without its corresponding reader. They are configured together, at once, they are effectively inseparable. The reason we have this problem in the specification is that every language has its own idioms -- we have a situation where the SDK calls the exporter and the exporter-or-the-reader calls the SDK. Because of this cycle, the setup is an implementation detail. It's OK with me if the reader delegates to the exporter, it's OK with me if the reader receives functional options to say the same thing, and it's OK with me if there's a factory involved in the setup. Note that we have not specified the Register() method that many SDKs are using to bind the reader--that's also an implementation detail.

If the group consensus is that metric exporters MUST be delegated to via a specified interface, then we should specify those interfaces. Java uses a TemporalitySelector and an AggregationSelector. However, I prefer to follow the library guidelines -- exporters should handle protocols, which means readers should handle configuration.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 19, 2022

Closing in favor of #2746. This indeed is a duplicate of that.

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 spec:metrics Related to the specification/metrics directory triaged-accepted The issue is triaged and accepted by the OTel community, one can proceed with creating a PR proposal
Projects
None yet
Development

No branches or pull requests

4 participants