-
Notifications
You must be signed in to change notification settings - Fork 651
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
Conversation
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py
Outdated
Show resolved
Hide resolved
If I could make a suggestion, the entrypoints for 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
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 |
That looks good for the auto instrumentation. How does this work If I am setting up metrics manually? |
@aabmass ...
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 |
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
so i imagine this was the intended behavior |
Would this only be for otlp exporters, or are you thinking of making any metric exporter configurable for temprality/aggregation? |
Any of them |
@aabmass @ocelotl @srikanthccv |
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py
Outdated
Show resolved
Hide resolved
I also noticed the docstring still mentions environment variables but doens't seem to check any opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py Lines 167 to 168 in 2d1bfb3
|
opentelemetry-sdk/tests/metrics/test_periodic_exporting_metric_reader.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #2792
Adds
preferred_temporality
andpreferred_aggregation
configuration to metrics exporter (which is currently only used byPeriodicExportingMetricReader
.Moved
OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE
configuration to OTLPMetricExporter.