OTel emitted metrics do not match behavior described in the docs

[kevinawoo]

The behavior described via SDK docs and how the the OTel package was written is different. Community Slack Discussion

According to these comments, the behavior is that the these metrics are monotone increasing.

• sdk-go/client/client.go

  Lines 1315 to 1316 in a921890

  	// MetricsCounter is an ever-increasing counter.
  	type MetricsCounter = metrics.Counter

• sdk-go/internal/common/metrics/handler.go

  Lines 49 to 53 in a921890

  	// Counter is an ever-increasing counter.
  	type Counter interface {
  	// Inc increments the counter value.
  	Inc(int64)
  	}

However the current OTel implementation supports explicitly supports negative numbers with the use of UpDownCounter

• sdk-go/contrib/opentelemetry/handler.go

  Lines 120 to 121 in a921890

  	func (m MetricsHandler) Counter(name string) client.MetricsCounter {
  	c, err := m.meter.Int64UpDownCounter(name)

• sdk-go/contrib/opentelemetry/handler_test.go

  Line 89 in a921890

  testCounter.Inc(-1)

The Bug

The ask is to either fix the docs, or change OTel's emitted metric to be a Counter so that it matches how it's more commonly used.

The UpDownCounter type doesn't make contextual sense for the metrics emitted by the Temporal SDK.

See the OTel-Contrib-Packages section below on how other SDKs use UpDownCounters vs Counters.

Specifications

• Version: v1.34.0

References

Temporal Go SDK

The introduction of OTel was introduced in #1336 (comment), with this comment left ambiguous about which counter type to use, however a test case was introduced to support negative numbers.

The summary of an internal discussion is that the package was built for general usage, without any limitations on the value.

Currently, all usages of the counter calls .Inc(1), however this may change, especially numbers > 1.

The behavior of the metrics was copied from the behavior of Tally.

Tally

uber-go/tally is ambiguous in which values is allowed, but it's implementation supports negative numbers:

• https://github.com/uber-go/tally/blob/12d200cf909b1094affc40bf4bf70fc99c5f39a3/stats.go#L72-L74
• There's no test cases that specifically address negative numbers https://github.com/uber-go/tally/blob/12d200cf909b1094affc40bf4bf70fc99c5f39a3/stats_test.go#L86-L101

Prometheus

Prometheus does not support negative numbers for counters and will panic if a negative number is provided. https://github.com/prometheus/client_golang/blob/9b83d994624f3cab82ec593133a598b3a27d0841/prometheus/counter.go#L126-L129

OTel Contrib Packages

OTel's Prometheus package implements counters as monotonic sums, which follows the spirit of Prometheus's implementation.

[A Counter] is a cumulative metric that represents a single monotonically increasing counter whose value can only increase or be reset to zero on restart.

-OTel's Prometheus Design Doc #Counters

• and it's code prometheusreceiver/internal/util.go

There's limited usage of UpDownCounters in all of the receiver packages. This metric is mostly commonly used to show the number of things in flight/active.

Note, I think Fluent's usage is actually a bug in how the code is auto generated.

23 occurrences (expand to see usage patterns)

Where the Counter type is used as a cumulative metric that represents a single monotonically increasing counter whose value can only increase or be reset to zero on restart.

Temporal SDK usage of `counter` metric type

Other SDK's use of `counter` type (164 occurrences)

[Sushisource]

@kevinawoo Yeah, so this is all objectively true, but I think the consequence is fairly low. We use gauges in other places, so any metrics collection system is going to need to support those (I know in the community thread, they mention their system doesn't handle gauges well - but what system would that be? There'd be a lot of metrics that are gauges now [and should be] that they would be unable to use), and although these particular metrics probably can and should be simple counters, the fact that they aren't isn't exactly causing any damage.

So with that in mind we discussed on the SDK team and we're not likely to take this on immediately. We have a broader effort tracked to normalize metrics across SDKs, I've parented this under that in Jira, and when we take on that effort we can decide if we'll do this then too.

[danielhochman]

Google Cloud Monitoring (formerly known as Stackdriver) doesn't natively support gauge deltas in their metrics query builder. This makes any sort of interactive debugging or exploration of metrics quite painful. Displaying a gauge is fine, just not deltas without writing MQL which is deprecated or their flavor of PromQL which is slightly different than PromQL, neither of which our team is trained up on.