Minor refactor MetricReader and AggregationTemporality

[reyang]

Trying to address the review comments / suggestions from @CodeBlanch.
Fixes #2355.

[codecov]

Codecov Report

Merging #2357 (b780244) into main (34d8332) will decrease coverage by 0.01%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##             main    #2357      +/-   ##
==========================================
- Coverage   80.96%   80.95%   -0.02%     
==========================================
  Files         229      229              
  Lines        7318     7323       +5     
==========================================
+ Hits         5925     5928       +3     
- Misses       1393     1395       +2     

Impacted Files	Coverage Δ
...orter.OpenTelemetryProtocol/OtlpMetricsExporter.cs	10.00% <ø> (ø)
...lemetry/Metrics/AggregationTemporalityAttribute.cs	0.00% <ø> (ø)
...Telemetry/Metrics/PeriodicExportingMetricReader.cs	0.00% <0.00%> (ø)
...OpenTelemetry/Metrics/BaseExportingMetricReader.cs	73.33% <100.00%> (+1.90%)	⬆️
src/OpenTelemetry/Metrics/MetricReader.cs	72.41% <100.00%> (ø)
...Telemetry/Internal/SelfDiagnosticsEventListener.cs	97.63% <0.00%> (+0.78%)	⬆️

[CodeBlanch]

I dropped some comments but nothing blocking.

[alanwest]

Not related to this PR, but I'm not following why the OTLP exporter is marked to prefer Cumulative. Seems to me the OTLP exporter should effectively not have a preference.

For a given service, is it currently not possible for some metrics to have a cumulative aggregation and others to have a delta aggregation?

[alanwest]

Ok, sorry, I think this is making sense to me now. I'm still trying to keep all these things in my head. I continue going back to a time when there was discussion about being able to define aggregation temporality at the level of an individual instrument or view. This is not the case.

Aggregation temporality is essentially defined at the level of a pipeline - so like MetricReader -> MetricExporter.

[reyang]

Let me put some concrete examples to help us align the understanding:

Prometheus - only supports Cumulative (implied that Cumulative is also the preference):

• The user doesn't have to specify temporality during the SDK configuration, everything will be exported as cumulative. In case the measurement being reported is Delta, there will be Delta->Cumulative conversion.
• If the user specified Cumulative explicitly, it won't harm
• If the user specified Delta explicitly, they will get some error telling them that the setup is wrong.

OTLP - supports both Cumulative and Delta (and Cumulative is the preference):

• The user doesn't have to specify temporality during the SDK configuration, everything will be exported as Cumulative. In case the measurement being reported is Delta, there will be Delta->Cumulative conversion.
• If the user specified Cumulative explicitly, it won't harm
• If the user specified Delta explicitly, there are two cases:
  • If the value being reported is Delta (e.g. Counter), it'll be reported as Delta, no conversion required
  • If the value being reported is Cumulative (e.g. ObservableCounter), there will be Cumulative->Delta conversion.

[reyang]

@alanwest FYI open-telemetry/opentelemetry-specification#731

[alanwest]

Thank you for this context! Defaulting to cumulative had seemed somewhat arbitrary to me, but Prometheus + load-balanced collectors = a bad time if delta. Prometheus seems to make everything trickier 😄.