Do not export counters with no points for OTLP.

[carlosalberto]

Potential fix for #1976

It shows that nothing seems to break with this simple change.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@28905b7). Click here to learn what that means.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##             master    #1978   +/-   ##
=========================================
  Coverage          ?   84.16%           
  Complexity        ?     1803           
=========================================
  Files             ?      215           
  Lines             ?     7325           
  Branches          ?      791           
=========================================
  Hits              ?     6165           
  Misses            ?      858           
  Partials          ?      302           

Impacted Files	Coverage Δ	Complexity Δ
.../io/opentelemetry/exporter/otlp/MetricAdapter.java	96.24% <0.00%> (ø)	31.00 <0.00> (?)
...lemetry/sdk/extension/otproto/TraceProtoUtils.java	76.19% <0.00%> (ø)	6.00% <0.00%> (?%)
...n/java/io/opentelemetry/api/common/Attributes.java	92.00% <0.00%> (ø)	18.00% <0.00%> (?%)
.../metrics/aggregator/DoubleLastValueAggregator.java	100.00% <0.00%> (ø)	7.00% <0.00%> (?%)
.../io/opentelemetry/sdk/trace/NoopSpanProcessor.java	100.00% <0.00%> (ø)	8.00% <0.00%> (?%)
...io/opentelemetry/sdk/trace/data/ImmutableLink.java	100.00% <0.00%> (ø)	5.00% <0.00%> (?%)
...lemetry/context/PersistentHashArrayMappedTrie.java	45.45% <0.00%> (ø)	4.00% <0.00%> (?%)
...opagation/B3PropagatorInjectorMultipleHeaders.java	100.00% <0.00%> (ø)	4.00% <0.00%> (?%)
.../opentelemetry/sdk/metrics/InstrumentRegistry.java	100.00% <0.00%> (ø)	6.00% <0.00%> (?%)
...telemetry/opentracingshim/NoopSpanBuilderShim.java	23.07% <0.00%> (ø)	2.00% <0.00%> (?%)
... and 206 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28905b7...af447fe. Read the comment docs.

[anuraaga]

LGTM - this seems like a reasonable interpretation of the proto to me

[jkwatson]

My only worry is there might be a reason to send an empty payload, just as a heartbeat. Other than that, this looks fine to me.

[carlosalberto]

@bogdandrutu Any take on this? 😉

[carlosalberto]

Marking this as Ready to Review, in order to get more eyes ;)

[jmacd]

I agree with this as a short-term fix that will make the Java OTLP Metrics export not send what to me is a meaningless point. It's uncovered that we should probably specify in the protocol that a Metric with empty point list may be regarded as equivalent to an absent Metric. If the specification says we can silently drop these Metrics with no data points, then we can assert that this is a correct change.

I would like to bring the Java Metric SDK into more conceptual alignment with the OTel-Go SDK. There we have a basic Processor component with two distinct kinds of support that are present in order to implement Prometheus and/or Cumulative metric export pipelines.

1. First, the Processor is the component that is responsible for converting DELTA inputs (from Counter instruments) into CUMULATIVE outputs (i.e., OTLP Metric Sums with AggregationTemporality=CUMULATIVE)
2. Second, the Processor has a "Memory" option that is responsible for maintaining state about every combination of metric instrument and label set that was ever encountered.

Both of these behaviors combine are needed for a Prometheus export pipeline. Only the first of these behaviors is needed for an OTLP/CUMULATIVE export pipeline. None of these behaviors is needed for an OTLP/DELTA export pipeline. Since I'm not familiar with the internals, I don't know how much restructuring is needed to accomplish this goal, but when it is accomplished, there will be a basic Processor component with two optional behaviors. I approve this PR with a TODO mentioning the final state and the need for more SDK specification.

Whether using a CUMULATIVE or DELTA strategy with OTLP, there is no reason to use empty data points. We should either not report a Metric ("no observation(s)") or we should report a Metric with more than zero points ("some observations").

There are independent questions raised here that should not block the resolution of the immediate issue. For example: if a Processor is configured for CUMULATIVE export and not MEMORY behavior, shall we omit those points because of a lack of observations? If the instrument is a SumObserver and the point that was previously observed is later not observed and then subsequently observed again, without changing the start timestamp: what should we make of this? I don't think it's too important what we say about this situation, but in any case we should not be sending Metric values with empty points, right?

@open-telemetry/specs-metrics-approvers ^^^ some questions for the Metrics specification.

@jkwatson Re: "to send an empty payload, just as a heartbeat". I would advocate for using the memory behavior described here if you need this kind of information. See also open-telemetry/opentelemetry-specification#1102.

[jmacd]

@bogdandrutu I'd like your opinion specifically. See also #1976 (comment)

[carlosalberto]

@bogdandrutu @jkwatson @jmacd I think we should merge this change, to keep things 'conservative'. Then we can iterate and make this selectable (in order to support Prometheus, for example). Let me know what you think.

[bogdandrutu]

As @jmacd pointed this is a good short term fix.