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

Fix metric export interval to match spec #2641

Conversation

svetlanabrennan
Copy link
Contributor

@svetlanabrennan svetlanabrennan commented Nov 19, 2021

The spec says export interval should be 60000 milliseconds and this pr fixes this for the otlp metric exporter.

Changes

Please provide a brief description of the changes here.

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

  • CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@svetlanabrennan svetlanabrennan requested a review from a team November 19, 2021 00:06
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 19, 2021

CLA Signed

The committers are authorized under a signed CLA.

/// </summary>
public int MetricExportIntervalMilliseconds { get; set; } = Timeout.Infinite;
public int MetricExportIntervalMilliseconds { get; set; } = 60000;
Copy link
Member

@reyang reyang Nov 19, 2021

Choose a reason for hiding this comment

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

Why would a console exporter use the periodic exporting strategy by default?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes that makes sense now. The side-effect of this change causing the console exporter to default to periodic exporting was not immediately obvious.

var reader = options.MetricExportIntervalMilliseconds == Timeout.Infinite
? new BaseExportingMetricReader(exporter)
: new PeriodicExportingMetricReader(exporter, options.MetricExportIntervalMilliseconds);

My gut says it would make sense to decouple the idea of periodic vs. non-periodic in a separate config setting. Kinda similar to how tracing has the batch exporter options.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe break this into two PRs... because I think the change to the OTLP exporter is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I broke it up.

@codecov
Copy link

codecov bot commented Nov 19, 2021

Codecov Report

Merging #2641 (9924dd3) into main (66d2621) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 9924dd3 differs from pull request most recent head 508a0fd. Consider uploading reports for the commit 508a0fd to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2641      +/-   ##
==========================================
- Coverage   80.97%   80.92%   -0.05%     
==========================================
  Files         243      243              
  Lines        8547     8547              
==========================================
- Hits         6921     6917       -4     
- Misses       1626     1630       +4     
Impacted Files Coverage Δ
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 100.00% <100.00%> (ø)
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 97.14% <0.00%> (-2.86%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 73.33% <0.00%> (-0.96%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 96.87% <0.00%> (-0.79%) ⬇️

@svetlanabrennan svetlanabrennan force-pushed the svetlanabrennan/change-metric-export-interval branch from 50fc91c to 74c02cb Compare November 19, 2021 01:10
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.

@cijothomas
Copy link
Member

@svetlanabrennan Could you update the description as it is outdated now? The linked issue require separate handling, so that may be removed.

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.

4 participants