[rhythm] Make ID generator more robust

[mapno]

What this PR does:

Improves the deterministic ID generator to allocate less memory and be robust to all kinds of inputs.

Probably not a fair comparison, but still:

Benchmark vs `uuid.New()`

goos: darwin
goarch: arm64
pkg: github.com/grafana/tempo/modules/blockbuilder/util
cpu: Apple M2 Pro
BenchmarkDeterministicID-12    	 8512278	       125.5 ns/op	     120 B/op	       4 allocs/op
BenchmarkDeterministicID-12    	 9522423	       124.7 ns/op	     120 B/op	       4 allocs/op
BenchmarkDeterministicID-12    	 9609878	       126.4 ns/op	     120 B/op	       4 allocs/op
BenchmarkDeterministicID-12    	 8931763	       136.2 ns/op	     120 B/op	       4 allocs/op
BenchmarkDeterministicID-12    	 8639715	       136.5 ns/op	     120 B/op	       4 allocs/op
BenchmarkDeterministicID-12    	 8680236	       146.6 ns/op	     120 B/op	       4 allocs/op
BenchmarkDeterministicID-12    	 8684743	       137.5 ns/op	     120 B/op	       4 allocs/op
BenchmarkDeterministicID-12    	 7703137	       140.7 ns/op	     120 B/op	       4 allocs/op
BenchmarkDeterministicID-12    	 8473578	       136.9 ns/op	     120 B/op	       4 allocs/op
BenchmarkDeterministicID-12    	 8839968	       149.4 ns/op	     120 B/op	       4 allocs/op
BenchmarkNewID-12              	 4698474	       256.1 ns/op	      16 B/op	       1 allocs/op
BenchmarkNewID-12              	 4671002	       255.4 ns/op	      16 B/op	       1 allocs/op
BenchmarkNewID-12              	 4707766	       255.3 ns/op	      16 B/op	       1 allocs/op
BenchmarkNewID-12              	 4633113	       257.3 ns/op	      16 B/op	       1 allocs/op
BenchmarkNewID-12              	 4654569	       257.4 ns/op	      16 B/op	       1 allocs/op
BenchmarkNewID-12              	 4631960	       260.3 ns/op	      16 B/op	       1 allocs/op
BenchmarkNewID-12              	 4648914	       257.1 ns/op	      16 B/op	       1 allocs/op
BenchmarkNewID-12              	 4283595	       257.9 ns/op	      16 B/op	       1 allocs/op
BenchmarkNewID-12              	 4703736	       254.7 ns/op	      16 B/op	       1 allocs/op
BenchmarkNewID-12              	 4499523	       256.0 ns/op	      16 B/op	       1 allocs/op
PASS
ok  	github.com/grafana/tempo/modules/blockbuilder/util	29.115s

[mdisibio]

Any reason not to call uuid.NewSHA1, since we are already using that hash function and version?

[mapno]

It's so hash.Hash is not created each time, saving some allocations.

[mdisibio]

The current global var isn't safe for concurrent use, how about making it a struct var? I know the ID generator isn't called concurrently yet, but there's nothing preventing it, and seems likely (parallelism in the block builder).

[joe-elliott]

I've been strongly preferring the fnv1a static methods that require no struct and have no concurrency concerns.

https://pkg.go.dev/github.com/segmentio/fasthash/fnv1a

[mapno]

The current global var isn't safe for concurrent use, how about making it a struct var?

Moved the hash.Hash to a struct field. Agree it's safer, and the difference won't be noticeable.

I've been strongly preferring the fnv1a static methods that require no struct and have no concurrency concerns.
https://pkg.go.dev/github.com/segmentio/fasthash/fnv1a

From what I read, the main arguments for using fasthash is saving allocs and avoiding inefficient string to bytes conversions.

We'd need to do the alloc manually since we're reusing the data byte slice between sequential IDs and already have bytes. I don't see a big benefit on using other than Go's sha1 in this case.

[joe-elliott]

i may be misunderstanding the issue, but hashing bytes does not alloc with fnv1a. i

https://github.com/segmentio/fasthash/blob/v1.0.3/fnv1a/hash.go#L76-L108

while working on ingester locking I benched it against this:

tempo/pkg/tracesizes/traceSizes.go

Lines 28 to 32 in e0e6e6c

	func (s *Tracker) token(traceID []byte) uint64 {
	s.hash.Reset()
	s.hash.Write(traceID)
	return s.hash.Sum64()
	}

neither function alloc'ed, but it's twice as fast, has no state and requires no locking. i think the only question is whether or not its hash alg is as collision resistant.

[mdisibio]

Yes agree that would be my concern with fnv. I think we can start with the SHAs in the uuid package and go from there. Realistically this is not called much (once per block), much less than trace/span hashing.