Mismatch of units between calculated span duration and metric name

[bw972]

Component(s)

connector/servicegraph, processor/servicegraph

What happened?

Description

The service graph connector calculates and buckets the duration of spans in milliseconds but stores it in a metric with the suffix _seconds.

Steps to Reproduce

Enable the service graph connector.

Expected Result

The buckets and calculated span durations should match the unit in the metric name.

Potential Solution

The following span duration calculations are modified to return the duration in seconds:

float64(span.EndTimestamp()-span.StartTimestamp()) / float64(time.Millisecond.Nanoseconds())

e.g.

float64(span.EndTimestamp()-span.StartTimestamp()) / float64(time.Second.Nanoseconds())

Additionally, the following default buckets to be updated to seconds:

	defaultLatencyHistogramBucketsMs = []float64{
		2, 4, 6, 8, 10, 50, 100, 200, 400, 800, 1000, 1400, 2000, 5000, 10_000, 15_000,
	}

e.g.

	defaultLatencyHistogramBucketsSec = []float64{
		0.002, 0.004, 0.006, 0.008, 0.01, 0.05, 0.1, 0.2, 0.4, 0.8, 1.0, 1.4, 2.0, 5.0, 10.0, 15.0,
	}

Collector version

v0.85.0

Environment information

No response

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

[github-actions]

Pinging code owners:

• connector/servicegraph: @jpkrohling @mapno
• processor/servicegraph: @jpkrohling @mapno

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

[crobert-1]

I agree this is a valid issue. From documentation, timestamps are epoch time in nano seconds, so we need to divide by time.Second.Nanosecond as you've shared rather than time.Second.Millisecond to match the unit in the name. Examples of incorrect calculation can be found in this method.

The math has been this way since the original introduction of the processor, so users likely expect the values to be in milliseconds even though it doesn't line up with the unit name. Due to this incorrect expectation, we need to clearly communicated to users that this change occurred so they're not caught off guard.

I'd think fixing the unit is a better solution than changing metric names to match implementation. In my mind milliseconds makes a bit more sense as the latency metric, but it would be a breaking change to update them. I'll defer to code owners regarding the best path forward.

[crobert-1]

Fixed by #27665