distributor: add counter for spans in the future

[carles-grafana]

What this PR does:

This will allow us to find customers that send spans in the future,
which can't be found using the Search API.

Which issue(s) this PR fixes:
Fixes #

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[javiermolinar]

I like the idea. I think this is quite similar to what we have in the data quality package. What do you think of adding a new reason to the existing metric there?. It would be good to have all the related code in the same place. The name of the metrics and the help could be improved, though..

tempo/pkg/dataquality/warnings.go

Lines 19 to 23 in 911e7e0

	var metric = promauto.NewCounterVec(prometheus.CounterOpts{
	Namespace: "tempo",
	Name: "warnings_total",
	Help: "The total number of warnings per tenant with reason.",
	}, []string{"tenant", "reason"})

[carles-grafana]

@javiermolinar I've converted it to the warn metric

[carles-grafana]

As discussed with Joe, we should use the slack time to determine whether a span is in the past or in the future. However, this configuration field is only available in the Ingesters, and these have RF3, which means we cannot reliably count ingested spans there.

I will try instead to implement it in the block builder for Tempo 3.

[carles-grafana]

In the end we decided to put it in the distributor as the implementation is simpler and we'll have real time data, which wouldn't be possible in the block-builder

[joe-elliott]

the suffix on prometheus metrics should be the unit. something like?

tempo_spans_distance_in_future_seconds

wdyt about putting these in the dataquality package:

https://github.com/grafana/tempo/blob/main/pkg/dataquality/warnings.go

[carles-grafana]

Changed the names.

[joe-elliott]

we use span end time to enforce ingestion slack in other spots. should we use the end time here? what are we attempting to do with this metric?

if the goal is to measure lag due to the tracing pipeline i think end makes more sense, but maybe our goals here are different?

[carles-grafana]

I don't think it's worth having the metric in the past, only for spans in the future, unless you have use case in mind

[carles-grafana]

which makes me thing a histogram isn't really worth it. Maybe we should go for the original idea of counting spans in the future and that's it.

[joe-elliott]

A spans in the future counter seems useful as it suggests something wildly wrong like a system clock misconfigured or a script that's generating bad data.

A spans in the past histogram seems useful as it can give information about the impact of delays due to trace pipelining.

@mapno how does rhythm impact ingestion slack in the block builder/generator? does the time in queue impact this behavior or do we propagate the ingestion time forward somehow?

[mapno]

@mapno how does rhythm impact ingestion slack in the block builder/generator? does the time in queue impact this behavior or do we propagate the ingestion time forward somehow?

Traces get longer to get to metrics-generators (and block-builders) than with the classic architecture. Delay should go from milliseconds in normal operation to whatever lag it accumulates—minutes, hours.

In the distributors there is no difference.

[carles-grafana]

I've changed it to the end time, I agree it makes more sense.

[knylander-grafana]

Are there any changes that impact Tempo users? If so, we should consider adding docs.

[carles-grafana]

@joe-elliott are we still interested in having this?

[joe-elliott]

Please fix the changelog. It's trying to add to 2.8 but I'm good on this.

Double check that you can get a decent visualization out of this and then let's talk internally about exposing it to cloud customers as a dataquality metric.