Additional default latency buckets for spanmetrics

[fredr]

What this PR does:
Changes the default spanmetric latency bucket sizes to match the default sizes in https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/spanmetricsprocessor

It will be hard to size these default buckets to a size that fits all systems, but I think wider is better than narrower for a default. The current default maxes out at 4 seconds which imo is a bit low.

Checklist

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

[CLAassistant]

All committers have signed the CLA.

[joe-elliott]

I am fine with this change. I think most people running default Tempo are doing so at low volumes and will benefit from the additional buckets. Pinging @kvrhdn and @mapno for thoughts. I am good with merging this if you are.

Is there a specific reason to not use exponential buckets as generated by Prometheus? If we just change the line to:

  cfg.HistogramBuckets = prometheus.ExponentialBuckets(0.002, 2, 14) // 12 -> 14

then we will get buckets:

  [0.002 0.004 0.008 0.016 0.032 0.064 0.128 0.256 0.512 1.024 2.048 4.096 8.192 16.384]

[fredr]

Is there a specific reason to not use exponential buckets as generated by Prometheus?

I copied the default buckets from https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/spanmetricsprocessor/processor.go#L51 thinking that there might be a value in having the same. But using the exponential buckets is definitely cleaner. I'll update the pr

[joe-elliott]

Thanks for the contribution! I am fine with this change and will approve, but will leave to @kvrhdn or @mapno to merge. They might have reasons to leave this out.

[mapno]

I'm fine with both options. Merging!

[mapno]

Agh, there are tests in the registry (registry_test.go) failing due to this change. Those will need to be fixed before we can merge.

[fredr]

Agh, there are tests in the registry (registry_test.go) failing due to this change. Those will need to be fixed before we can merge.

I've ran the whole test suite (make test) and ran TestManagedRegistry_removeStaleSeries locally on this branch, and it all pass. Could this be a flake? would it possible to re-run the test ci?

[fredr]

It failed again, but this time it was github.com/grafana/tempo/modules/distributor that didn't go through. Think that is unrelated to this change, but let me know if I should dig into it. (it also passes locally)

[yvrhdn]

Yeah, I'm also fine with this. Should we make the changelog more explicit about the additional buckets? This will increase active series of people using the defaults.
We aren't making the buckets itself wider, we are adding 2 more buckets.

[mapno]

Thanks for the patience. Merging!