Prometheus exporter should use Gauge for non-monotonic sums

[jmacd]

The existing exporters/metric/prometheus exporter exports a sum for the UpDown- instruments, which leads to issues like open-telemetry/opentelemetry-go-contrib#368.

The metric.Kind type already has a getter Kind.Monotonic() that tells us whether the Sum aggregation is an UpDown- variety or not. If the aggregation is UpDown-, Prometheus exporters should not export Counter data points, Prometheus exporters should export Gauge data points.

[Verlet64]

I'd like to pick up this issue, if that's okay @jmacd

[jmacd]

@Verlet64 yes please 😀

[Verlet64]

Amazing, thanks! Should be able to throw some time at this at the weekend.

[Verlet64]

Hey @jmacd,

I took a look at this on Sunday, and whilst I was able to pretty trivially make the export the data points as a gauge rather than a counter, by checking if the metric was monotonic or not, I feel like I'm missing something if we're talking fixing some of the related issues.

As as first pass, I was thinking of adding some logic to Collect in exporters/metric/prometheus/prometheus.go, something like the below:

if sum, ok := agg.(aggregation.Sum); ok && record.Descriptor().MetricKind().Monotonic() {
	if err := c.exportCounter(ch, sum, numberKind, desc, labels); err != nil {
		return fmt.Errorf("exporting counter: %w", err)
	}
} else if sum, ok := agg.(aggregation.Sum); ok && !record.Descriptor().MetricKind().Monotonic() {	
	if err := c.exportGauge(ch, sum, numberKind, desc, labels); err != nil {
		return fmt.Errorf("exporting counter: %w", err)
	}
}

And then the body of exportGauge being something like:

func (c *collector) exportGauge(ch chan<- prometheus.Metric, sum aggregation.Sum, kind metric.NumberKind, desc *prometheus.Desc, labels []string) error {
	v, err := sum.Sum()
	if err != nil {
		return fmt.Errorf("error retrieving counter: %w", err)
	}

	m, err := prometheus.NewConstMetric(desc, prometheus.GaugeValue, v.CoerceToFloat64(kind), labels...)
	if err != nil {
		return fmt.Errorf("error creating constant metric: %w", err)
	}

	ch <- m
	return nil
}

That would still result in the same observed behavior for something like a metric.Int64UpDownSumObserver - which makes sense, as we're still just emitting the aggregation Sum as a data point, which means we're only going to get an ever increasing measurement for the metric.

Off the back of this, I've got three questions:

1. Is there something with regards to the construction of a gauge value that I'm missing?
2. Is there another way you'd prefer this to be done?
3. In the linked issue is it correct to use a -SumObserver for things like memory allocation over time, or are we instead looking for snapshots of the current memory allocation, and therefore something more like a -ValueObserver?

Thanks!

[jmacd]

@Verlet64 your assumptions and conclusions are all correct. This issue calls for a minor change which you have spelled out perfectly. To your final question (3), yes, a UpDownSumObserver is the right instrument for memory allocation over time, since the measurement is a sum of bytes.

[jmacd]

I want to leave this open until it's stated in a specification somewhere.

[jmacd]

Now fixed by spec.