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

Respect OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE only for otlp exporter #2843

Merged
merged 18 commits into from
Aug 3, 2022

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Jul 26, 2022

Fixes #2792

Adds preferred_temporality and preferred_aggregation configuration to metrics exporter (which is currently only used by PeriodicExportingMetricReader.

Moved OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE configuration to OTLPMetricExporter.

@lzchen lzchen requested a review from a team July 26, 2022 20:24
@aabmass
Copy link
Member

aabmass commented Jul 28, 2022

If I could make a suggestion, the entrypoints for OTEL_METRIC_EXPORTERS should provide a MetricReader factory function. For example, for otlp that could be something like

def provide_reader() -> MetricReader:
  preferred_temporality=environ.get(
                    OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE,
                    "CUMULATIVE",
                )
                .upper()
                .strip()

  return PeriodicExportingMetricReader(
    exporter=OTLPExporter(),
    preferred_temporality=preferred_temporality,
    ...,
  )

then update

otlp_proto_grpc = opentelemetry.exporter.otlp.proto.grpc.metric_exporter:OTLPMetricExporter

to

 otlp_proto_grpc = opentelemetry.exporter.otlp.proto.grpc.metric_exporter:provide_reader

one downside of this approach is some code duplication across implementations of provide_reader() for other exporters if we have common environment variables to check. However it would simplify things for using MetricReaders like Prometheus which act as a "pull exporter", as they can easily implement this method as well. Right now im not sure how we would implement OTEL_METRICS_EXPORTER=prometheus

@srikanthccv
Copy link
Member

That looks good for the auto instrumentation. How does this work If I am setting up metrics manually?

@lzchen
Copy link
Contributor Author

lzchen commented Jul 28, 2022

@aabmass
Would the recommendation for users who manually setup their metrics pipeline with otlp exporter to use provide_reader?

...
reader = provide_reader()
provider = MeterProvider(metric_readers=[reader])
set_meter_provider(provider)
...

I'm also wondering how we will handle all the optional configuration needed for otlp exporter and the PeriodicExportingMetricReader, will we have all of these as optional parameters in provide_reader?

@aabmass
Copy link
Member

aabmass commented Jul 28, 2022

Would the recommendation for users who manually setup their metrics pipeline with otlp exporter to use provide_reader?

no that wasn't the intention, i see your point. The other option we discussed in SIG was having PeriodicExportingMetricReader defer to the paired exporter for returning the preferred temporality. What do you think of that? FWIW, the spec says

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. OpenTelemetry language implementations MAY support automatically configuring the MetricReader to use for an Exporter.

so i imagine this was the intended behavior

@lzchen
Copy link
Contributor Author

lzchen commented Jul 28, 2022

The other option we discussed in SIG was having PeriodicExportingMetricReader defer to the paired exporter for returning the preferred temporality.

Would this only be for otlp exporters, or are you thinking of making any metric exporter configurable for temprality/aggregation?

@aabmass
Copy link
Member

aabmass commented Jul 29, 2022

Any of them

@lzchen
Copy link
Contributor Author

lzchen commented Aug 1, 2022

@aabmass @ocelotl @srikanthccv
As discussed in the SIG, added preferred_temporality and preferred_aggregation configuration in the exporter. PeriodicExportingMetricReader will take into account exporter configuration first, and then its own configuration. The OTLP metric exporter also configures temporality via env var OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE.

@aabmass
Copy link
Member

aabmass commented Aug 2, 2022

I also noticed the docstring still mentions environment variables but doens't seem to check any

The value passed here will override the corresponding values set
via the environment variable

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE is used in MetricReader base class
4 participants