[connector/spanmetrics] Delta span metric StartTimeUnixNano doesn't follow specification, causing unbounded memory usage with prometheusexporter

[swar8080]

Component(s)

connector/spanmetrics

What happened?

Expected Behaviour

The specification on metric temporality expects uninterrupted series of delta data points to have timestamps like below:

Actual Behaviour

When configuring spanmetricsconnctor to use delta temporarily, the timestamps of successive data points follow the pattern (T1, T1), (T2, T2), (T3, T3). Basically the StartTimeUnixNano and TimeUnixNano are always set to the current timestamp.

When configured to use cumulative temporality, the connector caches StartTimeUnixNano so that it can be used in the next data point. However, when using delta temporality that cache (resourceMetrics) gets wiped after each round of exporting metrics:

opentelemetry-collector-contrib/connector/spanmetricsconnector/connector.go

Lines 283 to 301 in 71e6c55

	func (p *connectorImp) resetState() {
	// If delta metrics, reset accumulated data
	if p.config.GetAggregationTemporality() == pmetric.AggregationTemporalityDelta {
	p.resourceMetrics.Purge()
	p.metricKeyToDimensions.Purge()
	} else {
	p.resourceMetrics.RemoveEvictedItems()
	p.metricKeyToDimensions.RemoveEvictedItems()
	// Exemplars are only relevant to this batch of traces, so must be cleared within the lock
	if p.config.Histogram.Disable {
	return
	}
	p.resourceMetrics.ForEach(func(_ resourceKey, m *resourceMetrics) {
	m.histograms.Reset(true)
	})
	}
	}

Collector version

v0.94.0

Environment information

Testing done locally on mac OS

OpenTelemetry Collector configuration

connectors:
  spanmetrics:
    dimensions_cache_size: 5000
    resource_metrics_key_attributes:
      - service.name
    aggregation_temporality: "AGGREGATION_TEMPORALITY_DELTA"
    metrics_flush_interval: 5s
    exemplars:
      enabled: true
    histogram:
      explicit:
        buckets: ...

exporters:
  prometheus/exporter:
    endpoint: "0.0.0.0:9465"
    enable_open_metrics: true

service:
  pipelines:
    traces:
      receivers: [otlp]
      processors: []
      exporters: [spanmetrics]
    metrics:
      receivers: [otlp, spanmetrics]
      processors: []
      exporters: [prometheus/exporter]

Log output

No response

Additional context

Why fix this?

This could be a different way of fixing #30688 and #30559. It could also significantly reduce memory usage when combining spanmetricsconnector with exporters like prometheusexporter.

I'm currently seeing high memory use in the collectors at my work with this set-up

Issue with Prometheus Exporter

prometheusexporter supports conversion from delta to cumulative temporality. However, it only works if the series follows the specification:

opentelemetry-collector-contrib/exporter/prometheusexporter/accumulator.go

Lines 203 to 212 in 71e6c55

	// Delta-to-Cumulative
	if doubleSum.AggregationTemporality() == pmetric.AggregationTemporalityDelta && ip.StartTimestamp() == mv.value.Sum().DataPoints().At(0).Timestamp() {
	ip.SetStartTimestamp(mv.value.Sum().DataPoints().At(0).StartTimestamp())
	switch ip.ValueType() {
	case pmetric.NumberDataPointValueTypeInt:
	ip.SetIntValue(ip.IntValue() + mv.value.Sum().DataPoints().At(0).IntValue())
	case pmetric.NumberDataPointValueTypeDouble:
	ip.SetDoubleValue(ip.DoubleValue() + mv.value.Sum().DataPoints().At(0).DoubleValue())
	}
	}

I think the author's rationale in the above code is that the delta series seems reset based on this part of the spec. So with the current delta span metric behaviour, prometheus views each data point as if the counter was reset.

A workaround is having the connector produce cumulative span metrics, but that requires the connector caching all series in memory, which is redundant since prometheusexporter already does this. @matej-g is working on an improvement to expire infrequently updated series from the cache which will definitely help with reducing memory.

Solution Discussion

Having delta span metrics follow the spec could significantly reduce memory usage when used with prometheusexporter.

To do this, the connector probably has to cache the StartTimeUnixNano for each series when in delta mode. The memory used to cache the tunestamps should be much less than what resourceMetrics caches though. That cache's values are nested structs holding data like metric attributes and histogram bucket sizes

[github-actions]

Pinging code owners:

• connector/spanmetrics: @portertech

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[swar8080]

I forked the collector in our internal gitlab to apply the patch I had in mind. It's making a noticeable difference to memory usage:

I'll clean-up the change and open a PR to see if it makes sense to include here too

[mx-psi]

I think we should change the behavior of the connector, but to be clear: having start timestamp equal the point timestamp is valid per the spec:

When StartTimeUnixNano equals TimeUnixNano, a new unbroken sequence of observations begins with a reset at an unknown start time. The initial observed value is recorded to indicate that an unbroken sequence of observations resumes. These points have zero duration, and indicate that nothing is known about previously-reported points and that data may have been lost.

[crobert-1]

Looks like it's agreed that this is a bug, and a PR is welcome. I'll remove needs triage.

[github-actions]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• connector/spanmetrics: @portertech @Frapschen

See Adding Labels via Comments if you do not have permissions to add labels yourself.