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 11 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
7 changes: 7 additions & 0 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@

## Unreleased

* Fixed OTLP metric exporter to default to a periodic 60 second export cycle.
A bug was introduced in #2717 that caused the OTLP metric export to default
to a manual export cycle (i.e., requiring an explicit flush). A workaround
for this bug has been provided
[here](https://github.com/open-telemetry/opentelemetry-dotnet/issues/2979#issuecomment-1061060541).
([#2982](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2982))

## 1.2.0-rc3

Released 2022-Mar-04
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
2 changes: 1 addition & 1 deletion src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ OpenTelemetry.Metrics.PeriodicExportingMetricReader
OpenTelemetry.Metrics.PeriodicExportingMetricReader.PeriodicExportingMetricReader(OpenTelemetry.BaseExporter<OpenTelemetry.Metrics.Metric> exporter, int exportIntervalMilliseconds = 60000, int exportTimeoutMilliseconds = 30000) -> void
OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions
OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions.PeriodicExportingMetricReaderOptions() -> void
OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds.get -> int
OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds.get -> int?
OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds.set -> void
OpenTelemetry.ReadOnlyTagCollection
OpenTelemetry.ReadOnlyTagCollection.Count.get -> int
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ OpenTelemetry.Metrics.PeriodicExportingMetricReader
OpenTelemetry.Metrics.PeriodicExportingMetricReader.PeriodicExportingMetricReader(OpenTelemetry.BaseExporter<OpenTelemetry.Metrics.Metric> exporter, int exportIntervalMilliseconds = 60000, int exportTimeoutMilliseconds = 30000) -> void
OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions
OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions.PeriodicExportingMetricReaderOptions() -> void
OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds.get -> int
OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds.get -> int?
OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds.set -> void
OpenTelemetry.ReadOnlyTagCollection
OpenTelemetry.ReadOnlyTagCollection.Count.get -> int
Expand Down
5 changes: 5 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

## Unreleased

* The `PeriodicExportingMetricReader` now accepts an
`ExportIntervalMilliseconds` of `-1` indicating an infinite export interval
period.
([#2982](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2982))

## 1.2.0-rc3

Released 2022-Mar-04
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
3 changes: 2 additions & 1 deletion src/OpenTelemetry/Metrics/PeriodicExportingMetricReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ public PeriodicExportingMetricReader(
int exportTimeoutMilliseconds = DefaultExportTimeoutMilliseconds)
: base(exporter)
{
Guard.ThrowIfOutOfRange(exportIntervalMilliseconds, min: 1);
Guard.ThrowIfInvalidTimeout(exportIntervalMilliseconds);
Guard.ThrowIfZero(exportIntervalMilliseconds);
Guard.ThrowIfOutOfRange(exportTimeoutMilliseconds, min: 0);

if ((this.SupportedExportModes & ExportModes.Push) != ExportModes.Push)
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; }
}
}
34 changes: 33 additions & 1 deletion test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpMetricsExporterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,40 @@

namespace OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests
{
public class OtlpMetricsExporterTests
public class OtlpMetricsExporterTests : Http2UnencryptedSupportTests
{
[Fact]
public void TestAddOtlpExporter_SetsCorrectMetricReaderDefaults()
{
if (Environment.Version.Major == 3)
{
// Adding the OtlpExporter creates a GrpcChannel.
// This switch must be set before creating a GrpcChannel when calling an insecure HTTP/2 endpoint.
// See: https://docs.microsoft.com/aspnet/core/grpc/troubleshoot#call-insecure-grpc-services-with-net-core-client
AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true);
}

var meterProvider = Sdk.CreateMeterProviderBuilder()
.AddOtlpExporter()
.Build();

var bindingFlags = System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance;

var metricReader = typeof(MetricReader)
.Assembly
.GetType("OpenTelemetry.Metrics.MeterProviderSdk")
.GetField("reader", bindingFlags)
.GetValue(meterProvider) as PeriodicExportingMetricReader;

Assert.NotNull(metricReader);

var exportIntervalMilliseconds = (int)typeof(PeriodicExportingMetricReader)
.GetField("exportIntervalMilliseconds", bindingFlags)
.GetValue(metricReader);

Assert.Equal(60000, exportIntervalMilliseconds);
}

[Fact]
public void UserHttpFactoryCalled()
{
Expand Down