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.

Sign up for GitHub

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
+64 −45
Merged

Default to periodic metric reader for push based exporters #2982

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
56d32d7
Default push metric exporters to use periodic metric reader
alanwest Mar 7, 2022
51495d6
Define default export interval for push metric exporters
alanwest Mar 7, 2022
4afb66f
Update public API
alanwest Mar 7, 2022
08514a1
Update changelog
alanwest Mar 7, 2022
45b46cb
Allow infinite export interval
alanwest Mar 7, 2022
1a53185
Add test to validate Alan doesn't screw up AddOtlpExporter again
alanwest Mar 8, 2022
48fcc9d
Derive test class from Http2UnencryptedSupportTests
alanwest Mar 8, 2022
4990303
Set Http2UnencryptedSupport switch for netcoreapp3.1
alanwest Mar 8, 2022
dcd2e01
Merge branch 'main' into alanwest/default-periodic
cijothomas Mar 8, 2022
b126cde
Update SDK changelog
alanwest Mar 8, 2022
9475043
Merge branch 'alanwest/default-periodic' of github.com:alanwest/opent…
alanwest Mar 8, 2022
6f2cdea
Merge branch 'main' into alanwest/default-periodic
alanwest Mar 9, 2022
c9cbdab
Move default export interval to const
alanwest Mar 9, 2022
e87fdc2
Merge branch 'main' into alanwest/default-periodic
cijothomas Mar 10, 2022
6ab72a7
Merge branch 'main' into alanwest/default-periodic
cijothomas Mar 10, 2022
fde279f
Merge branch 'main' into alanwest/default-periodic
cijothomas Mar 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions src/OpenTelemetry.Exporter.Console/ConsoleExporterMetricsExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,11 @@ private static MeterProviderBuilder AddConsoleExporter(

var metricExporter = new ConsoleMetricExporter(exporterOptions);

if (metricReaderOptions.MetricReaderType == (MetricReaderType)(-1))
{
metricReaderOptions.MetricReaderType = MetricReaderType.Manual;
}

var metricReader = metricReaderOptions.MetricReaderType == MetricReaderType.Manual
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
? new BaseExportingMetricReader(metricExporter)
: new PeriodicExportingMetricReader(metricExporter, metricReaderOptions.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds);
: new PeriodicExportingMetricReader(
metricExporter,
metricReaderOptions.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds ?? -1);

metricReader.Temporality = metricReaderOptions.Temporality;
return builder.AddReader(metricReader);
Expand Down
9 changes: 3 additions & 6 deletions src/OpenTelemetry.Exporter.InMemory/InMemoryExporterMetricsExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,11 @@ private static MeterProviderBuilder AddInMemoryExporter(

var metricExporter = new InMemoryExporter<Metric>(exportedItems);

if (metricReaderOptions.MetricReaderType == (MetricReaderType)(-1))
{
metricReaderOptions.MetricReaderType = MetricReaderType.Manual;
}

var metricReader = metricReaderOptions.MetricReaderType == MetricReaderType.Manual
? new BaseExportingMetricReader(metricExporter)
: new PeriodicExportingMetricReader(metricExporter, metricReaderOptions.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds);
: new PeriodicExportingMetricReader(
metricExporter,
metricReaderOptions.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds ?? -1);

metricReader.Temporality = metricReaderOptions.Temporality;
return builder.AddReader(metricReader);
Expand Down
9 changes: 3 additions & 6 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,11 @@ private static MeterProviderBuilder AddOtlpExporter(

var metricExporter = new OtlpMetricExporter(exporterOptions);

if (metricReaderOptions.MetricReaderType == (MetricReaderType)(-1))
{
metricReaderOptions.MetricReaderType = MetricReaderType.Periodic;
}

var metricReader = metricReaderOptions.MetricReaderType == MetricReaderType.Manual
? new BaseExportingMetricReader(metricExporter)
: 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.


metricReader.Temporality = metricReaderOptions.Temporality;
return builder.AddReader(metricReader);
Expand Down
23 changes: 2 additions & 21 deletions src/OpenTelemetry/Metrics/MetricReaderOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,35 +21,16 @@ namespace OpenTelemetry.Metrics;
/// </summary>
public class MetricReaderOptions
{
private const MetricReaderType MetricReaderTypeUnspecified = (MetricReaderType)(-1);
private MetricReaderType metricReaderType = MetricReaderTypeUnspecified;

/// <summary>
/// Gets or sets the AggregationTemporality used for Histogram
/// and Sum metrics.
/// </summary>
public AggregationTemporality Temporality { get; set; } = AggregationTemporality.Cumulative;

/// <summary>
/// Gets or sets the <see cref="MetricReaderType" /> to use. Defaults to <c>MetricReaderType.Manual</c>.
/// Gets or sets the <see cref="MetricReaderType" /> to use. Defaults to <c>MetricReaderType.Periodic</c>.
/// </summary>
public MetricReaderType MetricReaderType
{
get
{
if (this.metricReaderType == MetricReaderTypeUnspecified)
{
this.metricReaderType = MetricReaderType.Manual;
}

return this.metricReaderType;
}

set
{
this.metricReaderType = value;
}
}
public MetricReaderType MetricReaderType { get; set; } = MetricReaderType.Periodic;

/// <summary>
/// Gets or sets the <see cref="PeriodicExportingMetricReaderOptions" /> options. Ignored unless <c>MetricReaderType</c> is <c>Periodic</c>.
Expand Down
4 changes: 2 additions & 2 deletions src/OpenTelemetry/Metrics/PeriodicExportingMetricReaderOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ namespace OpenTelemetry.Metrics
public class PeriodicExportingMetricReaderOptions
{
/// <summary>
/// Gets or sets the metric export interval in milliseconds. The default value is <c>60000</c>.
/// Gets or sets the metric export interval in milliseconds.
/// </summary>
public int ExportIntervalMilliseconds { get; set; } = 60000;
public int? ExportIntervalMilliseconds { get; set; }
}
}