ingest: make Enconding sync.Pool more robust

[carles-grafana]

We were resetting the length or the underlying slices but not clearing the contents.
This makes a potential future data leak due to a bug less likely.

Also add a test to ensure the data is reset when reusing it.

Benchmark in this branch:

goos: darwin
goarch: arm64
pkg: github.com/grafana/tempo/pkg/ingest
cpu: Apple M3 Pro
BenchmarkEncoderPoolPutDifferentSizes/Small_Original-11         	80590712	        15.56 ns/op	       0 B/op	       0 allocs/op
BenchmarkEncoderPoolPutDifferentSizes/Small_WithClear-11        	48544262	        25.38 ns/op	       0 B/op	       0 allocs/op
BenchmarkEncoderPoolPutDifferentSizes/Medium_Original-11        	19074123	        62.93 ns/op	       0 B/op	       0 allocs/op
BenchmarkEncoderPoolPutDifferentSizes/Medium_WithClear-11       	10028392	       117.1 ns/op	       0 B/op	       0 allocs/op
BenchmarkEncoderPoolPutDifferentSizes/Large_Original-11         	 1676859	       702.9 ns/op	       0 B/op	       0 allocs/op
BenchmarkEncoderPoolPutDifferentSizes/Large_WithClear-11        	 1000000	      1120 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/grafana/tempo/pkg/ingest	9.330s

Clearing the slices is significantly slower. (60-85% slower). Is the extra safety worth it?

What this PR does:

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]

[carles-grafana]

@mdisibio Inlining the reset function didn't provide any significant speedup

[joe-elliott]

For the benchmarks to be meaningful you need to run them a decent number of times in this branch and in main and compare the results.

Something like:

go test -run xxx -bench <your bench> -count=10 -benchmem > before.txt
go test -run xxx -bench <your bench> -count=10 -benchmem > after.txt
benchstat before.txt after.txt

[carles-grafana]

For the benchmarks to be meaningful you need to run them a decent number of times in this branch and in main and compare the results.

Something like:

go test -run xxx -bench <your bench> -count=10 -benchmem > before.txt
go test -run xxx -bench <your bench> -count=10 -benchmem > after.txt
benchstat before.txt after.txt

Note that in the new benchmark I'm already comparing the new and old implementation of encoderPoolPut (the original is reimplemented in the test file so I don't have to switch branches).

For the existing EncodeDecode benchmark, the effect is not visible:

  benchstat old.txt new.txt                                                    
  goos: darwin
  goarch: arm64
  pkg: github.com/grafana/tempo/pkg/ingest
  cpu: Apple M3 Pro
                  │   old.txt    │            new.txt            │
                  │    sec/op    │   sec/op     vs base          │
  EncodeDecode-11   139.0µ ± 23%   136.0µ ± 8%  ~ (p=0.684 n=10)

                  │   old.txt    │            new.txt             │
                  │     B/op     │     B/op      vs base          │
  EncodeDecode-11   1.242Mi ± 1%   1.255Mi ± 3%  ~ (p=0.796 n=10)

                  │   old.txt   │             new.txt             │
                  │  allocs/op  │  allocs/op   vs base            │
  EncodeDecode-11   2.004k ± 0%   2.004k ± 0%  ~ (p=1.000 n=10) ¹
  ¹ all samples are equal

[joe-elliott]

cleanup?

[carles-grafana]

This is an intentional implementation of the original function for the benchmark below.
Do you think it's not worth keeping?

[joe-elliott]

that's not unprecedented. here we keep the original implementation of truncateRowNumber:

https://github.com/grafana/tempo/blob/main/pkg/parquetquery/iters.go#L80-L88

but we also do this b/c it allows us to validate the more complex implementation against the original:

https://github.com/grafana/tempo/blob/main/pkg/parquetquery/iters_test.go#L28

i'm not opposed to this and will go ahead and approve, but imo this is a simple enough change that we are good removing the old impl