tsdb/wlog: Only treat unknown record types as failure

[aknuds1]

I noticed that tsdb/wlog.Watcher.readSegmentForGC treats e.g. histogram records as unsupported, and counts them against w.recordDecodeFailsMetric. I propose that we instead do this only for the record.Unknown type.

[machine424]

Good catch!

[machine424]

I didn’t grasp this comment from the initial reading, does it mean?

Suggested change

	// Could be corruption, or reading from a WAL from a newer Prometheus.
	// Could be corruption, or version incompatibility between the WAL and Prometheus.

[aknuds1]

I would prefer to keep the original comment personally, since it's from before this PR. To me it's also perfectly understandable at least.

[machine424]

The only minor downside that I can anticipate is that if we introduce a new type in the future that necessitates special handling here and we neglect to include it, recordsReadMetric (via a dashboard or an alert, if applicable) will no longer be capable of alerting us to the oversight. However, I believe we’ll eventually figure that out in another way.

[aknuds1]

Yeah, I don't see any problem with defaulting to ignore records at least, since the logic is explicitly to only handle series. It's better to not reimplement dec.Type's logic for detecting supported record types.

[aknuds1]

Thinking about it some more, is it at all possible certain record types (MmapMarkers and Metadata, I assume that HistogramSamples and FloatHistogramSamples were just not added to this logic) should not be encountered here?

[aknuds1]

@bboreham I think you wrote this code, can you clarify?

[bboreham]

I believe the code was copy-pasted from

prometheus/tsdb/wlog/watcher.go

Line 549 in 6f595c6

switch dec.Type(rec) {

, with processing for non-series removed.

I haven't been involved in work to add histograms or metadata.

More broadly, I would like the WAL to be a bit more backwards-compatible, e.g. to have a version number so we know what we're supposed to be doing, but I couldn't see a way to add that backwards-compatibly.

[bboreham]

I would expect the same change at

prometheus/tsdb/wlog/watcher.go

Lines 657 to 659 in 6f595c6

	default:
	// Could be corruption, or reading from a WAL from a newer Prometheus.
	w.recordDecodeFailsMetric.Inc()

[aknuds1]

I would expect the same change at

@bboreham thanks, I didn't notice the similarity. I can make a corresponding change there (if it makes sense, will have to think it through).

[bboreham]

How does this PR relate to #13181 ?

[aknuds1]

How does this PR relate to #13181 ?

I'll try to review that PR, wasn't known to me.

[aknuds1]

How does this PR relate to #13181 ?

@bboreham after reviewing that PR, as far as I can tell these two PRs are orthogonal to each other.

[aknuds1]

I would expect the same change at

@bboreham I've made the change also there, PTAL.