enhancement: supported deduping spans within block builder

[zhxiaogg]

What this PR does:

• Block builder: deduplicate spans within traces during block creation and track removed duplicates via tempo_block_builder_spans_deduped_total metric

Which issue(s) this PR fixes:
Fixes #6516

Test

• can verify the metrics are generated and showing in prometheus ui when testing with singlebinary example.

Checklist

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

[javiermolinar]

I wonder if we should use this hashing algorithm instead:

tempo/pkg/model/trace/combine.go

Line 27 in 2ad734b

func tokenForID(h hash.Hash64, buffer []byte, kind int32, b []byte) token {

It seems more correct, with less chances of collisions

[zhxiaogg]

thanks, changed to use the alternative one you suggested!

[stoewer]

To avoid heap allocations and traversing each trace twice, I'd suggest to remove dedupeTrace() and implement it's functionality inside the loop that updates timestamp bounds. Roughly like this:

seen := make(map[uint64]struct{}, 1024) // initialize seen before the outer `for entries := range seq` loop

...
for _, rs := range tr.ResourceSpans {
    for _, ss := range rs.ScopeSpans {
        unique := ss.Spans[:0]
        for _, s := range ss.Spans {
            // dedup and update timestamps 
        }
        ss.Spans = unique
    }
}
clear(seen)

While this is less readable than having a separate dedup function, it might still be worth it for performance reasons. What do you think?

[stoewer]

Nice in-place dedup 👍

[stoewer]

There is an opportunity here to save lots of smaller heap allocations:

• Initialize seen with a reasonable start size in to avoid allocations and rehashing when the map grows
• Reuse the seen in multiple deduplications (and use clear(seen) before reuse)

[zhxiaogg]

thanks, all changed including the following comments.

[stoewer]

Each trace is traversed twice: one time in dedupeTrace() and another time in L118 to update timestamp bounds. Maybe those can be combined

[stoewer]

The iterator must be exhausted before this can be accessed

Just for the sake of being a bit more defensive, what do you think about enforcing this by returning an error when liveTracesIter.DedupedSpans() is called before it's exhausted?

[javiermolinar]

Looks good to me