agent db: make rejecting ooo samples configurable

[rabenhorst]

Add storage.agent.reject-out-of-order-samples flag and make rejecting out-of-order samples in agent's db configurable. This allows disabling remote writing out-of-order samples from agent, which was introduced in #12897.

[ArthurSens]

Maybe I'm missing something, but isn't it already possible to disable OOO ingestion with the configuration file?

From my understanding if out_of_order_time_window is zero (the default), then OOO is disabled. See https://prometheus.io/docs/prometheus/latest/configuration/configuration/#tsdb

[rabenhorst]

From my understanding if out_of_order_time_window is zero (the default), then OOO is disabled. See https://prometheus.io/docs/prometheus/latest/configuration/configuration/#tsdb

Maybe I miss something, but I don't see how this applies to the agent db, which, as I understand, is just the TSDB WAL.

For context, the use-case here is to avoid remote writing ooo samples (e.g. when the sink would reject them anyhow).

[sdufel]

When agents ingest out-of-order samples and and remote write them to external stores, it can be extremely expensive on the other end. We run a setup with prometheus agents writing to thanos receivers, and out of order samples generate severe amounts of load.

[ArthurSens]

Maybe I miss something, but I don't see how this applies to the agent db, which, as I understand, is just the TSDB WAL.

Yeah, I'm reading the code now and it looks like the agent indeed doesn't take this into consideration.

I think that creating yet another configuration option for out of order would be confusing though. What do you think about re-using the already existing out_of_order_time_window configuration option?

[rabenhorst]

Yeah, I'm reading the code now and it looks like the agent indeed doesn't take this into consideration.

I think that creating yet another configuration option for out of order would be confusing though. What do you think about re-using the already existing out_of_order_time_window configuration option?

I think it makes sense and I changed it and added some test cases for edge cases. Is this what you had in mind?

It might be confusing now that out_of_order_time_window, which is part of tsdb config, is also applied to agent db. What would be the best place to document it?

[ArthurSens]

It LGTM, just a few nitpicks

I'll try to ping maintainers to take a look at this as well

[ArthurSens]

I'm also realizing that changing the default behavior now (always accept OOO -> have to configure window to accept) might be a breaking change. Since agent-mode is a feature flag and OOO is marked as experimental in our docs, I think the change should be fine, but let's see what others think

[ArthurSens]

It might be confusing now that out_of_order_time_window, which is part of tsdb config, is also applied to agent db. What would be the best place to document it?

Good point 😬, I don't remember if the configuration documentation is auto-generated... I'm 90% sure it's not. So it should be fine to just add a statement here clarifying that it is also applied for agent-mode?

[rabenhorst]

It LGTM, just a few nitpicks

I'll try to ping maintainers to take a look at this as well

I addressed the nits and thx!

[roidelapluie]

cc @rfratto

[jesusvazquez]

Looks good.

I think rejecting OOO doing remote write is a fair choice regardless or whether or not the backend can ingest it. But its even a more valid choice given today's context where OOO is still not enabled by default.

Left a small nit that I'd like to see fixed for consistency 👍

Nice work 💪

[jesusvazquez]

I'd rename this method to minValidTime or appendable() to make it consistent with oither parts of the codebase

prometheus/tsdb/head_append.go

Lines 453 to 455 in d699dc3

	func (s *memSeries) appendable(t int64, v float64, headMaxt, minValidTime, oooTimeWindow int64) (isOOO bool, oooDelta int64, err error) {
	// Check if we can append in the in-order chunk.
	if t >= minValidTime {

[rabenhorst]

Done, renamed to minValidTime.

[jesusvazquez]

Holding off merge for a @rfratto review since he maintains the agent 👍

[tpaschalis]

Since we're reusing an existing Prometheus config field, should we update docs for out_of_order_time_window around https://prometheus.io/docs/prometheus/latest/configuration/configuration/#tsdb to mention this dual use-case?

[rabenhorst]

Since we're reusing an existing Prometheus config field, should we update docs for out_of_order_time_window around https://prometheus.io/docs/prometheus/latest/configuration/configuration/#tsdb to mention this dual use-case?

I added it to out_of_order_time_window docs:

prometheus/docs/configuration/configuration.md

Lines 3805 to 3807 in 017b43e

	# When out_of_order_time_window is greater than 0, it also affects experimental agent. It allows
	# the agent's WAL to accept out-of-order samples that fall within the specified time window relative
	# to the timestamp of the last appended sample for the same series.

WDYT?

[ArthurSens]

Since the next release process starts next week, I'd merge this even without @rfratto's approval 😬.

@rabenhorst, it seems like you forgot to sign your lastest commit. Could you fix that please? 🙂