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

Default to periodic metric reader for push based exporters #2982

Merged
merged 16 commits into from
Mar 11, 2022

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Mar 7, 2022

Fixes #2979.

The OTLP exporter was mistakingly defaulted to the manual metric reader.

With this PR:

  • The periodic reader is reestablished as the default for the OTLP exporter with a default interval of 60 seconds.
  • The Console and InMemory exporters now default to using the periodic reader, but with an infinite interval by default (i.e., effectively the same as manual). There will be a spec PR to follow to define what the default interval should be for Console and InMemory exporters.
  • As a consequence, the PeriodicExportingMetricReader now allows an infinite export interval.

@alanwest alanwest requested a review from a team March 7, 2022 21:20
@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #2982 (fde279f) into main (5f509bf) will increase coverage by 0.59%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2982      +/-   ##
==========================================
+ Coverage   84.13%   84.72%   +0.59%     
==========================================
  Files         258      258              
  Lines        9093     9095       +2     
==========================================
+ Hits         7650     7706      +56     
+ Misses       1443     1389      -54     
Impacted Files Coverage Δ
...rter.InMemory/InMemoryExporterMetricsExtensions.cs 68.00% <100.00%> (+4.00%) ⬆️
...tryProtocol/Implementation/MetricItemExtensions.cs 93.79% <100.00%> (+0.26%) ⬆️
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs 71.42% <100.00%> (+7.14%) ⬆️
src/OpenTelemetry/Metrics/MetricReaderOptions.cs 100.00% <100.00%> (+25.00%) ⬆️
...Telemetry/Metrics/PeriodicExportingMetricReader.cs 76.47% <100.00%> (+76.47%) ⬆️
...ry/Metrics/PeriodicExportingMetricReaderOptions.cs 100.00% <100.00%> (ø)
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs 86.53% <0.00%> (-2.89%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 73.68% <0.00%> (-1.76%) ⬇️
...OpenTelemetry/Metrics/BaseExportingMetricReader.cs 86.95% <0.00%> (+1.44%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+2.94%) ⬆️
... and 6 more

: new PeriodicExportingMetricReader(metricExporter, metricReaderOptions.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds);
: new PeriodicExportingMetricReader(
metricExporter,
metricReaderOptions.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds ?? 60000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Idea - Might be good to have the default outside of this extension in a constant somewhere; but I believe it is already defined somewhere else so using that is OK as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to a constant. The constant already defined is part of the SDK project and is internal so not accessible here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually come to think of it the right thing to do here is to just do new PeriodicExportingMetricReader(metricExporter);. I've got a follow up PR to remove MetricReaderType. I'll clean that up in that PR.

@cijothomas cijothomas merged commit 8ca6574 into open-telemetry:main Mar 11, 2022
@alanwest alanwest deleted the alanwest/default-periodic branch March 23, 2022 16:23
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.

Metrics are no longer flushed automatically
4 participants