Bryan Boreham
Bryan Boreham
Discussed at the bug scrub. I will take a closer look.
I would expect the same change at https://github.com/prometheus/prometheus/blob/6f595c6762d253c0ef7574b93f9795a6e3e40c81/tsdb/wlog/watcher.go#L657-L659
How does this PR relate to #13181 ?
> treats e.g. histogram records as unsupported Can you clarify this for me please: I see `record.HistogramSamples` and `record.FloatHistogramSamples` as cases in the switch statement.
Oh I get it, this was actually a bug in `readSegmentForGC` only, and my "I would expect" comment caused you to make it a bigger bug. Sorry about that.
@roidelapluie the whole idea of this PR is to avoid comparing the regexp as a string, since "never set" and "set to a string equal to the default" are not...
Why do we need both an explicit string and a regex, given the former is a subset of the latter? Incidentally "pattern" did not immediately suggest "regex" to me, maybe...
Discussed again at the bug scrub; seems like a useful change. @LeviHarrison since you looked through it could you add a test please?
Please add `-benchmem` to the benchmark. Also check that benchmark calls the function you are changing.
> How do you wish to proceed from here? Write a valid benchmark, that does the same amount of work per iteration.