MetricReader TemporalityPreference #3153
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3153 +/- ##
==========================================
+ Coverage 84.75% 84.78% +0.03%
==========================================
Files 260 259 -1
Lines 9273 9295 +22
==========================================
+ Hits 7859 7881 +22
Misses 1414 1414
|
|
@joaopgrassi @utpilla tagging folks whom I know have written own MetricExporters, and will need to adjust when this PR is merged. |
|
@MikeGoldsmith in case you need.. (I saw some HoneyComb MetricExporter..) |
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
| Cumulative = 1, | ||
|
|
||
| /// <summary> | ||
| /// All measurements that are monotnic in nature are aggregated using delta temporality. |
There was a problem hiding this comment.
@alanwest nit: "monotnic" -> "monotonic"
| /// All measurements that are monotnic in nature are aggregated using delta temporality. | ||
| /// Aggregations of non-monotonic measurements use cumulative temporality. | ||
| /// </summary> | ||
| Delta = 2, |
There was a problem hiding this comment.
@alanwest Thinking out loud here, could we be more descriptive with the names? Like...
AlwaysCumulative,
DeltaMonotonicCumulativeNonmonotonicThere was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oops sorry my response was redundant... Is it just me or does GitHub not seem to refresh stuff reliably?
There was a problem hiding this comment.
Is it just me or does GitHub not seem to refresh stuff reliably?
Same here, I keep seeing such issue.
There was a problem hiding this comment.
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";
}There was a problem hiding this comment.
+1 on being explicit about naming.
BTW, which one is more idiomatic in .NET OpenTelemetry.Metrics.MetricReaderTemporalityPreference or OpenTelemetry.Metrics.MetricReader.TemporalityPreference (inner class)?
This PR introduces a new enumeration called
MetricReaderTemporalityPreferenceand provides it as a setting onMetricReader.MetricReaderno longer contains aTemporalityconfiguration setting.MetricReaderTemporalityPreferenceembodies the two preferences outlined in the specification of the OTLP exporter. Other preferences may be introduced in the future - for example, this table suggests a potential future "stateless" temporality preference.For the purposes of this PR, a "temporality preference" is effectively a mapping of instrument type (e.g., Counter, Histogram, AsyncCounter, etc) to an
AggregationTemporality(i.e., either Cumulative or Delta). This mapping will become important with the introduction of theUpDownCounterin .NET 7.The proposed names of the preferences and the corresponding mappings from instrument type to
AggregationTemporalityare as followsCumulativepreferenceMonotonicDelta* preferenceCounter<>ObservableCounter<>UpDownCounter<>ObservableUpDownCounter<>Histogram<>ObservableGauge<>MonotonicDeltabecause simplyDeltamay be misleading. The idea is that this preference uses delta temporality for all but the "non-monotonic" instruments. Furthermore, there may be a future where a trueDeltapreference may be needed - though as of yet, no one has specified a need for such a thing.With the removal of the
MetricReader.Temporalitysetting, I also deleted theAggregationTemporalityAttributeclass. This was only being used by the Prometheus package. It should only be possible to configure aMetricReaderpaired with the Prometheus exporter withMetricReaderTemporalityPreference.Cumulative. This attribute was enforcing this. My thought is that a stable version of the Prometheus package will come later. At that time maybe we can reintroduce anMetricReaderTemporalityPreferenceAttributeclass to enforce this, or maybe we will take another approach. RemovingAggregationTemporalityAttributefor now gives us more options to explore after the 1.2 release.