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

MetricReader TemporalityPreference #3153

Merged
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
4 changes: 2 additions & 2 deletions examples/Console/TestMetrics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ internal static object Run(MetricsOptions options)
exporterOptions.Protocol = options.UseGrpc ? OtlpExportProtocol.Grpc : OtlpExportProtocol.HttpProtobuf;

metricReaderOptions.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds = options.DefaultCollectionPeriodMilliseconds;
metricReaderOptions.Temporality = options.IsDelta ? AggregationTemporality.Delta : AggregationTemporality.Cumulative;
metricReaderOptions.TemporalityPreference = options.IsDelta ? MetricReaderTemporalityPreference.Delta : MetricReaderTemporalityPreference.Cumulative;
});
}
else
Expand All @@ -82,7 +82,7 @@ internal static object Run(MetricsOptions options)
exporterOptions.Targets = ConsoleExporterOutputTargets.Console;

metricReaderOptions.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds = options.DefaultCollectionPeriodMilliseconds;
metricReaderOptions.Temporality = options.IsDelta ? AggregationTemporality.Delta : AggregationTemporality.Cumulative;
metricReaderOptions.TemporalityPreference = options.IsDelta ? MetricReaderTemporalityPreference.Delta : MetricReaderTemporalityPreference.Cumulative;
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ namespace OpenTelemetry.Exporter
/// <summary>
/// Exporter of OpenTelemetry metrics to Prometheus.
/// </summary>
[AggregationTemporality(AggregationTemporality.Cumulative)]
[ExportModes(ExportModes.Pull)]
public class PrometheusExporter : BaseExporter<Metric>, IPullMetricExporter
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ private static MeterProviderBuilder AddPrometheusExporter(MeterProviderBuilder b

var exporter = new PrometheusExporter(options);
var reader = new BaseExportingMetricReader(exporter);
reader.TemporalityPreference = MetricReaderTemporalityPreference.Cumulative;

return builder.AddReader(reader);
}
Expand Down
14 changes: 7 additions & 7 deletions src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,14 @@ OpenTelemetry.Metrics.ExplicitBucketHistogramConfiguration
OpenTelemetry.Metrics.ExplicitBucketHistogramConfiguration.Boundaries.get -> double[]
OpenTelemetry.Metrics.ExplicitBucketHistogramConfiguration.Boundaries.set -> void
OpenTelemetry.Metrics.ExplicitBucketHistogramConfiguration.ExplicitBucketHistogramConfiguration() -> void
OpenTelemetry.Metrics.MetricReader.TemporalityPreference.get -> OpenTelemetry.Metrics.MetricReaderTemporalityPreference
OpenTelemetry.Metrics.MetricReader.TemporalityPreference.set -> void
OpenTelemetry.Metrics.MetricReaderOptions
OpenTelemetry.Metrics.MetricReaderOptions.MetricReaderOptions() -> void
OpenTelemetry.Metrics.MetricReaderOptions.PeriodicExportingMetricReaderOptions.get -> OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions
OpenTelemetry.Metrics.MetricReaderOptions.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.MetricReaderOptions.Temporality.set -> void
OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.AggregationTemporality.Cumulative = 1 -> OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.AggregationTemporality.Delta = 2 -> OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.AggregationTemporalityAttribute
OpenTelemetry.Metrics.AggregationTemporalityAttribute.AggregationTemporalityAttribute(OpenTelemetry.Metrics.AggregationTemporality temporality) -> void
OpenTelemetry.Metrics.AggregationTemporalityAttribute.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.BaseExportingMetricReader
OpenTelemetry.Metrics.BaseExportingMetricReader.BaseExportingMetricReader(OpenTelemetry.BaseExporter<OpenTelemetry.Metrics.Metric> exporter) -> void
OpenTelemetry.Metrics.BaseExportingMetricReader.SupportedExportModes.get -> OpenTelemetry.Metrics.ExportModes
Expand Down Expand Up @@ -76,8 +73,11 @@ OpenTelemetry.Metrics.MetricReader.Collect(int timeoutMilliseconds = -1) -> bool
OpenTelemetry.Metrics.MetricReader.Dispose() -> void
OpenTelemetry.Metrics.MetricReader.MetricReader() -> void
OpenTelemetry.Metrics.MetricReader.Shutdown(int timeoutMilliseconds = -1) -> bool
OpenTelemetry.Metrics.MetricReader.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.MetricReader.Temporality.set -> void
OpenTelemetry.Metrics.MetricReaderOptions.TemporalityPreference.get -> OpenTelemetry.Metrics.MetricReaderTemporalityPreference
OpenTelemetry.Metrics.MetricReaderOptions.TemporalityPreference.set -> void
OpenTelemetry.Metrics.MetricReaderTemporalityPreference
OpenTelemetry.Metrics.MetricReaderTemporalityPreference.Cumulative = 1 -> OpenTelemetry.Metrics.MetricReaderTemporalityPreference
OpenTelemetry.Metrics.MetricReaderTemporalityPreference.Delta = 2 -> OpenTelemetry.Metrics.MetricReaderTemporalityPreference
OpenTelemetry.Metrics.MetricStreamConfiguration
OpenTelemetry.Metrics.MetricStreamConfiguration.Description.get -> string
OpenTelemetry.Metrics.MetricStreamConfiguration.Description.set -> void
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,14 @@ OpenTelemetry.Metrics.ExplicitBucketHistogramConfiguration
OpenTelemetry.Metrics.ExplicitBucketHistogramConfiguration.Boundaries.get -> double[]
OpenTelemetry.Metrics.ExplicitBucketHistogramConfiguration.Boundaries.set -> void
OpenTelemetry.Metrics.ExplicitBucketHistogramConfiguration.ExplicitBucketHistogramConfiguration() -> void
OpenTelemetry.Metrics.MetricReader.TemporalityPreference.get -> OpenTelemetry.Metrics.MetricReaderTemporalityPreference
OpenTelemetry.Metrics.MetricReader.TemporalityPreference.set -> void
OpenTelemetry.Metrics.MetricReaderOptions
OpenTelemetry.Metrics.MetricReaderOptions.MetricReaderOptions() -> void
OpenTelemetry.Metrics.MetricReaderOptions.PeriodicExportingMetricReaderOptions.get -> OpenTelemetry.Metrics.PeriodicExportingMetricReaderOptions
OpenTelemetry.Metrics.MetricReaderOptions.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.MetricReaderOptions.Temporality.set -> void
OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.AggregationTemporality.Cumulative = 1 -> OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.AggregationTemporality.Delta = 2 -> OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.AggregationTemporalityAttribute
OpenTelemetry.Metrics.AggregationTemporalityAttribute.AggregationTemporalityAttribute(OpenTelemetry.Metrics.AggregationTemporality temporality) -> void
OpenTelemetry.Metrics.AggregationTemporalityAttribute.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.BaseExportingMetricReader
OpenTelemetry.Metrics.BaseExportingMetricReader.BaseExportingMetricReader(OpenTelemetry.BaseExporter<OpenTelemetry.Metrics.Metric> exporter) -> void
OpenTelemetry.Metrics.BaseExportingMetricReader.SupportedExportModes.get -> OpenTelemetry.Metrics.ExportModes
Expand Down Expand Up @@ -76,8 +73,11 @@ OpenTelemetry.Metrics.MetricReader.Collect(int timeoutMilliseconds = -1) -> bool
OpenTelemetry.Metrics.MetricReader.Dispose() -> void
OpenTelemetry.Metrics.MetricReader.MetricReader() -> void
OpenTelemetry.Metrics.MetricReader.Shutdown(int timeoutMilliseconds = -1) -> bool
OpenTelemetry.Metrics.MetricReader.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality
OpenTelemetry.Metrics.MetricReader.Temporality.set -> void
OpenTelemetry.Metrics.MetricReaderOptions.TemporalityPreference.get -> OpenTelemetry.Metrics.MetricReaderTemporalityPreference
OpenTelemetry.Metrics.MetricReaderOptions.TemporalityPreference.set -> void
OpenTelemetry.Metrics.MetricReaderTemporalityPreference
OpenTelemetry.Metrics.MetricReaderTemporalityPreference.Cumulative = 1 -> OpenTelemetry.Metrics.MetricReaderTemporalityPreference
OpenTelemetry.Metrics.MetricReaderTemporalityPreference.Delta = 2 -> OpenTelemetry.Metrics.MetricReaderTemporalityPreference
OpenTelemetry.Metrics.MetricStreamConfiguration
OpenTelemetry.Metrics.MetricStreamConfiguration.Description.get -> string
OpenTelemetry.Metrics.MetricStreamConfiguration.Description.set -> void
Expand Down
13 changes: 13 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,19 @@

## Unreleased

* Removed the `Temporality` setting on `MetricReader` and replaced it with
`TemporalityPreference`. This is a breaking change.
`TemporalityPreference` is used to determine the `AggregationTemporality`
used on a per-instrument kind basis. Currently, there are two preferences:
* `Cumulative`: Measurements from all instrument kinds are aggregated using
`AggregationTemporality.Cumulative`.
* `Delta`: Measurements from `Counter`, `ObservableCounter`, and `Histogram`
instruments are aggregated using `AggregationTemporality.Delta`. When
UpDownCounters are supported with
[DiagnosticSource version 7.0 onwards](https://www.nuget.org/packages/System.Diagnostics.DiagnosticSource/7.0.0-preview.2.22152.2),
they will be aggregated using `AggregationTemporality.Cumulative`.
([#3153](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3153))

* Fix issue where `ExplicitBucketHistogramConfiguration` could be used to
configure metric streams for instruments that are not histograms. Currently,
it is not possible to change the aggregation of an instrument with views. This
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ internal static PeriodicExportingMetricReader CreatePeriodicExportingMetricReade

var metricReader = new PeriodicExportingMetricReader(exporter, exportInterval, exportTimeout)
{
Temporality = options.Temporality,
TemporalityPreference = options.TemporalityPreference,
};

return metricReader;
Expand Down
33 changes: 0 additions & 33 deletions src/OpenTelemetry/Metrics/AggregationTemporalityAttribute.cs

This file was deleted.

9 changes: 1 addition & 8 deletions src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,7 @@ public BaseExportingMetricReader(BaseExporter<Metric> exporter)
this.exporter = exporter;

var exportorType = exporter.GetType();
var attributes = exportorType.GetCustomAttributes(typeof(AggregationTemporalityAttribute), true);
if (attributes.Length > 0)
{
var attr = (AggregationTemporalityAttribute)attributes[attributes.Length - 1];
this.Temporality = attr.Temporality;
}

attributes = exportorType.GetCustomAttributes(typeof(ExportModesAttribute), true);
var attributes = exportorType.GetCustomAttributes(typeof(ExportModesAttribute), true);
if (attributes.Length > 0)
{
var attr = (ExportModesAttribute)attributes[attributes.Length - 1];
Expand Down
58 changes: 49 additions & 9 deletions src/OpenTelemetry/Metrics/MetricReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

using System;
using System.Diagnostics;
using System.Diagnostics.Metrics;
using System.Threading;
using System.Threading.Tasks;
using OpenTelemetry.Internal;
Expand All @@ -27,35 +28,74 @@ namespace OpenTelemetry.Metrics
/// </summary>
public abstract partial class MetricReader : IDisposable
{
private const AggregationTemporality AggregationTemporalityUnspecified = (AggregationTemporality)0;
private const MetricReaderTemporalityPreference MetricReaderTemporalityPreferenceUnspecified = (MetricReaderTemporalityPreference)0;

private static Func<Type, AggregationTemporality> cumulativeTemporatlityPreferenceFunc =
(instrumentType) => AggregationTemporality.Cumulative;

private static Func<Type, AggregationTemporality> monotonicDeltaTemporatlityPreferenceFunc = (instrumentType) =>
{
return instrumentType.GetGenericTypeDefinition() switch
{
var type when type == typeof(Counter<>) => AggregationTemporality.Delta,
var type when type == typeof(ObservableCounter<>) => AggregationTemporality.Delta,
var type when type == typeof(Histogram<>) => AggregationTemporality.Delta,

// Temporatlity is not defined for gauges, so this does not really affect anything.
var type when type == typeof(ObservableGauge<>) => AggregationTemporality.Delta,

// With .NET 7 the OpenTelemetry .NET SDK will support UpDownCounters.
// These will be aggregated using Cumulative temporatlity.
// See:
// https://docs.microsoft.com/dotnet/api/system.diagnostics.metrics.updowncounter-1
// https://docs.microsoft.com/dotnet/api/system.diagnostics.metrics.observableupdowncounter-1
// var type when type == typeof(UpDownCounter<>) => AggregationTemporality.Cumulative,
// var type when type == typeof(ObservableUpDownCounter<>) => AggregationTemporality.Cumulative,

// TODO: Consider logging here because we should not fall through to this case.
_ => AggregationTemporality.Delta,
};
};

private readonly object newTaskLock = new();
private readonly object onCollectLock = new();
private readonly TaskCompletionSource<bool> shutdownTcs = new();
private AggregationTemporality temporality = AggregationTemporalityUnspecified;
private MetricReaderTemporalityPreference temporalityPreference = MetricReaderTemporalityPreferenceUnspecified;
private Func<Type, AggregationTemporality> temporatlityFunc = cumulativeTemporatlityPreferenceFunc;
private int shutdownCount;
private TaskCompletionSource<bool> collectionTcs;
private BaseProvider parentProvider;

public AggregationTemporality Temporality
public MetricReaderTemporalityPreference TemporalityPreference
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
{
get
{
if (this.temporality == AggregationTemporalityUnspecified)
if (this.temporalityPreference == MetricReaderTemporalityPreferenceUnspecified)
{
this.temporality = AggregationTemporality.Cumulative;
this.temporalityPreference = MetricReaderTemporalityPreference.Cumulative;
}

return this.temporality;
return this.temporalityPreference;
}

set
{
if (this.temporality != AggregationTemporalityUnspecified)
if (this.temporalityPreference != MetricReaderTemporalityPreferenceUnspecified)
{
throw new NotSupportedException($"The temporality cannot be modified (the current value is {this.temporality}).");
throw new NotSupportedException($"The temporality preference cannot be modified (the current value is {this.temporalityPreference}).");
}

this.temporality = value;
this.temporalityPreference = value;
switch (value)
{
case MetricReaderTemporalityPreference.Delta:
this.temporatlityFunc = monotonicDeltaTemporatlityPreferenceFunc;
break;
case MetricReaderTemporalityPreference.Cumulative:
default:
this.temporatlityFunc = cumulativeTemporatlityPreferenceFunc;
break;
}
}
}

Expand Down
9 changes: 7 additions & 2 deletions src/OpenTelemetry/Metrics/MetricReaderExt.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ public abstract partial class MetricReader
private Metric[] metricsCurrentBatch;
private int metricIndex = -1;

internal AggregationTemporality GetAggregationTemporality(Type instrumentType)
{
return this.temporatlityFunc(instrumentType);
}

internal Metric AddMetricWithNoViews(Instrument instrument)
{
var metricStreamIdentity = new MetricStreamIdentity(instrument, metricStreamConfiguration: null);
Expand Down Expand Up @@ -66,7 +71,7 @@ internal Metric AddMetricWithNoViews(Instrument instrument)
Metric metric = null;
try
{
metric = new Metric(metricStreamIdentity, this.Temporality, this.maxMetricPointsPerMetricStream);
metric = new Metric(metricStreamIdentity, this.GetAggregationTemporality(metricStreamIdentity.InstrumentType), this.maxMetricPointsPerMetricStream);
}
catch (NotSupportedException nse)
{
Expand Down Expand Up @@ -152,7 +157,7 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric
else
{
Metric metric;
metric = new Metric(metricStreamIdentity, this.Temporality, this.maxMetricPointsPerMetricStream, metricStreamIdentity.HistogramBucketBounds, metricStreamIdentity.TagKeys);
metric = new Metric(metricStreamIdentity, this.GetAggregationTemporality(metricStreamIdentity.InstrumentType), this.maxMetricPointsPerMetricStream, metricStreamIdentity.HistogramBucketBounds, metricStreamIdentity.TagKeys);

this.instrumentIdentityToMetric[metricStreamIdentity] = metric;
this.metrics[index] = metric;
Expand Down
5 changes: 2 additions & 3 deletions src/OpenTelemetry/Metrics/MetricReaderOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@ namespace OpenTelemetry.Metrics;
public class MetricReaderOptions
{
/// <summary>
/// Gets or sets the AggregationTemporality used for Histogram
/// and Sum metrics.
/// Gets or sets the <see cref="MetricReaderTemporalityPreference" />.
/// </summary>
public AggregationTemporality Temporality { get; set; } = AggregationTemporality.Cumulative;
public MetricReaderTemporalityPreference TemporalityPreference { get; set; } = MetricReaderTemporalityPreference.Cumulative;

/// <summary>
/// Gets the <see cref="PeriodicExportingMetricReaderOptions" /> options.
Expand Down
35 changes: 35 additions & 0 deletions src/OpenTelemetry/Metrics/MetricReaderTemporalityPreference.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// <copyright file="MetricReaderTemporalityPreference.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

namespace OpenTelemetry.Metrics;

/// <summary>
/// Defines the behavior of a <see cref="MetricReader" />
/// with respect to <see cref="AggregationTemporality" />.
/// </summary>
public enum MetricReaderTemporalityPreference
{
/// <summary>
/// All aggregations are performed using cumulative temporatlity.
/// </summary>
Cumulative = 1,

/// <summary>
/// All measurements that are monotnic in nature are aggregated using delta temporality.
Copy link
Member

Choose a reason for hiding this comment

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

@alanwest nit: "monotnic" -> "monotonic"

Copy link
Member Author

Choose a reason for hiding this comment

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

#3162 💥

/// Aggregations of non-monotonic measurements use cumulative temporality.
/// </summary>
Delta = 2,
Copy link
Member

Choose a reason for hiding this comment

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

@alanwest Thinking out loud here, could we be more descriptive with the names? Like...

AlwaysCumulative,
DeltaMonotonicCumulativeNonmonotonic

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk_exporters/otlp.md The OTLP ENV variable called this simply cumulative, delta.
Though I agree a more descriptive name is nicer, I also prefer if we keep it close to the spec. (its really an OTLP spec, but thats the only place spec ever mentions this...)

Copy link
Member Author

@alanwest alanwest Apr 13, 2022

Choose a reason for hiding this comment

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

They are totally waaay more descriptive 😆. I originally had them as Cumulative and MonotonicDelta, but the spec has no such term. In discussing with @cijothomas, we decided to keep the names from the OTLP spec used for the OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE environment variable - https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk_exporters/otlp.md. We have not yet implemented this environment variable, but the meaning of the values CUMULATIVE and DELTA of this variable will map to this new enum.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops sorry my response was redundant... Is it just me or does GitHub not seem to refresh stuff reliably?

Copy link
Member

Choose a reason for hiding this comment

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

Is it just me or does GitHub not seem to refresh stuff reliably?

Same here, I keep seeing such issue.

Copy link
Member

Choose a reason for hiding this comment

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

I personally don't have an issue with the environment variable values being different from the enumeration names, but if that's what you guys want to do, fine by me. Could also use string constants instead of enum, I don't think the numeric values are significant to the implementation but I didn't look at every file on this PR. Eg:

public static class MetricReaderTemporalityPreference
{
   public const string AlwaysCumulative = "Cumulative";
   public const string DeltaMonotonicCumulativeNonmonotonic = "Delta";
}

Copy link
Member

Choose a reason for hiding this comment

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

+1 on being explicit about naming.

BTW, which one is more idiomatic in .NET OpenTelemetry.Metrics.MetricReaderTemporalityPreference or OpenTelemetry.Metrics.MetricReader.TemporalityPreference (inner class)?

}
Loading