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

Clean up metric reader options #3038

Merged

Conversation

alanwest
Copy link
Member

  • Removes MetricReaderType enumeration.
  • Adds PeriodicExportingMetricReaderOptions.ExportTimeoutMilliseconds with a default of 30000 for all push exporters.
  • MetricReaderOptions.PeriodicExportingMetricReaderOptions is no longer settable.

@alanwest alanwest requested a review from a team March 12, 2022 20:39
@codecov
Copy link

codecov bot commented Mar 12, 2022

Codecov Report

Merging #3038 (21798c0) into main (0254d34) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3038      +/-   ##
==========================================
+ Coverage   84.74%   84.91%   +0.16%     
==========================================
  Files         258      259       +1     
  Lines        9105     9112       +7     
==========================================
+ Hits         7716     7737      +21     
+ Misses       1389     1375      -14     
Impacted Files Coverage Δ
...rter.InMemory/InMemoryExporterMetricsExtensions.cs 66.66% <100.00%> (-1.34%) ⬇️
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs 70.37% <100.00%> (-1.06%) ⬇️
...ry/Internal/PeriodicExportingMetricReaderHelper.cs 100.00% <100.00%> (ø)
src/OpenTelemetry/Metrics/MetricReaderOptions.cs 100.00% <100.00%> (ø)
...Telemetry/Metrics/PeriodicExportingMetricReader.cs 76.47% <100.00%> (ø)
...ry/Metrics/PeriodicExportingMetricReaderOptions.cs 100.00% <100.00%> (ø)
...heus/Implementation/PrometheusCollectionManager.cs 79.74% <0.00%> (-2.54%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 90.41% <0.00%> (+3.65%) ⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 77.27% <0.00%> (+18.18%) ⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 78.57% <0.00%> (+28.57%) ⬆️

@@ -25,6 +26,9 @@ namespace OpenTelemetry.Metrics
/// </summary>
public static class ConsoleExporterMetricsExtensions
{
private const int DefaultExportIntervalMilliseconds = Timeout.Infinite;
private const int DefaultExportTimeoutMilliseconds = 30000;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need timeout 30000 or Infinite?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just submitted open-telemetry/opentelemetry-specification#2415 for further clarification.

I'll set to Infinite for now and change depending on how things shake out.

@@ -26,6 +27,9 @@ namespace OpenTelemetry.Metrics
/// </summary>
public static class InMemoryExporterMetricsExtensions
{
private const int DefaultExportIntervalMilliseconds = Timeout.Infinite;
private const int DefaultExportTimeoutMilliseconds = 30000;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need timeout 30000 or Infinite?

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 except the same qn as Reiley about default for ExportTimeout to be infinite vs 30

@utpilla
Copy link
Contributor

utpilla commented Mar 14, 2022

We could get rid of the default exportInterval and timeout in PeriodicExportingMetricReader now as we would always be providing the parameter values for the PeriodicExportingMetricReader ctor:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Metrics/PeriodicExportingMetricReader.cs#L31-L32

@alanwest
Copy link
Member Author

We could get rid of the default exportInterval and timeout in PeriodicExportingMetricReader now as we would always be providing the parameter values for the PeriodicExportingMetricReader ctor:

It is still possible to instantiate the PeriodicExportingMetricReader by itself, so the defaults are still relevant. Something like

var metricExporter = new ConsoleExporter();
var metricReader = new PeriodicExportingMetricReader(exporter);
var meterProviderBuilder = Sdk.CreateMeterProviderBuilder().AddReader(metricReader);

@cijothomas cijothomas merged commit a709cfd into open-telemetry:main Mar 15, 2022
@alanwest alanwest deleted the alanwest/remove-metric-reader-type branch March 15, 2022 18:19
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