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

Support OTEL_METRIC_EXPORT_INTERVAL and OTEL_METRIC_EXPORT_TIMEOUT #3424

Merged
merged 7 commits into from
Jul 15, 2022

Conversation

pellared
Copy link
Member

@pellared pellared commented Jul 1, 2022

Fixes #3417.

Changes

The MetricReaderOptions defaults can be overridden using
OTEL_METRIC_EXPORT_INTERVAL and OTEL_METRIC_EXPORT_TIMEOUT
environmental variables as defined in the
specification.

This change applies to all metrics exporters that use PeriodicExportingMetricReaderHelper.CreatePeriodicExportingMetricReader

  • OTLP
  • Console
  • InMemory

I think it is not possible to implement it for all instances of PeriodicExportingMetricReader without changing and breaking its public API (assuming that the value in "options" has precedence over the environmental variable which is the currently followed pattern).

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed
    • I added default values to CreatePeriodicExportingMetricReader that match the OTel spec

@pellared pellared requested a review from a team July 1, 2022 08:17
@codecov
Copy link

codecov bot commented Jul 1, 2022

Codecov Report

Merging #3424 (0a0d734) into main (bc4f788) will increase coverage by 0.10%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3424      +/-   ##
==========================================
+ Coverage   86.20%   86.30%   +0.10%     
==========================================
  Files         263      263              
  Lines        9545     9550       +5     
==========================================
+ Hits         8228     8242      +14     
+ Misses       1317     1308       -9     
Impacted Files Coverage Δ
...Telemetry/Metrics/PeriodicExportingMetricReader.cs 78.18% <75.00%> (+5.45%) ⬆️
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs 80.76% <100.00%> (-1.38%) ⬇️
...ry/Internal/PeriodicExportingMetricReaderHelper.cs 100.00% <100.00%> (ø)
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️
src/OpenTelemetry/Metrics/MetricReader.cs 88.13% <0.00%> (+1.69%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+5.88%) ⬆️
src/OpenTelemetry/ProviderExtensions.cs 90.90% <0.00%> (+9.09%) ⬆️
...tation/OpenTelemetryProtocolExporterEventSource.cs 85.00% <0.00%> (+10.00%) ⬆️

@pellared pellared added pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package metrics Metrics signal related enhancement New feature or request labels Jul 1, 2022
@pellared pellared requested a review from RassK July 5, 2022 13:43
@pellared
Copy link
Member Author

pellared commented Jul 6, 2022

Side note:

These failures are awkward: https://github.com/open-telemetry/opentelemetry-dotnet/runs/7217170744?check_suite_focus=true

Did anyone encounter the same problem? Looks like some strange side-effect - I have not analyzed it.

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

Looks good to me :) Just left one small question about the readme changes.

@pellared pellared requested a review from joaopgrassi July 8, 2022 14:27
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.

@reyang
Copy link
Member

reyang commented Jul 11, 2022

The environment variables are still marked as Experimental
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md, what's the plan here (e.g. push the change to a feature branch; wait until the spec becomes stable, merge in the main branch and do not release stable until the spec becomes stable)?

reyang
reyang previously requested changes Jul 11, 2022
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

@cijothomas
Copy link
Member

The environment variables are still marked as Experimental https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md, what's the plan here (e.g. push the change to a feature branch; wait until the spec becomes stable, merge in the main branch and do not release stable until the spec becomes stable)?

Did not realize that https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#periodic-exporting-metricreader was still experimental!

Will 1st check if Metrics Spec is close to marking this stable, and based on that device a plan for support in the SDK.

@pellared
Copy link
Member Author

pellared commented Jul 11, 2022

@cijothomas @reyang
These env vars are to my best knowledge already supported by OTel Java, Python, and PHP. See here and here. I think the probability that this part of the spec would change is close to 0. Of course, double-checking with Metrics SIG would be good 😉

I guess that OTEL_METRICS_EXEMPLAR_FILTER env var is the "more experimental" env var. This is the only reason why I am not creating a PR in the OTel spec 😉

EDIT: I decided to create open-telemetry/opentelemetry-specification#2658

@reyang reyang added the needs-spec-change Issues which require the OpenTelemetry Specification to clarify or define behavior label Jul 11, 2022
@reyang reyang dismissed their stale review July 15, 2022 05:47

Spec issue resolved.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@pellared pellared removed the needs-spec-change Issues which require the OpenTelemetry Specification to clarify or define behavior label Jul 15, 2022
@cijothomas cijothomas merged commit a19d106 into open-telemetry:main Jul 15, 2022
@pellared pellared deleted the OTEL_METRIC_EXPORT-env-vars branch July 15, 2022 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request metrics Metrics signal related pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support OTEL_METRIC_EXPORT_INTERVAL and OTEL_METRIC_EXPORT_TIMEOUT
7 participants