Add an experimental flag to block samples with timestamp too far in the future

[jnyi]

Hi Team,

TL; DR when we run thanos receiver, sometimes 1 sample with a bad timestamp too far in the future could pollute TSDB head and block other valid samples even with tsdb.out-of-order.time-window enabled, for example like 10 years in the future. Prometheus implementation didn't have a good way to deal with this issue neither: https://github.com/prometheus/prometheus/blob/main/tsdb/head_append.go#L401

Added this tsdb.too-far-in-future.time-window flag will prevent samples with timestamp > current + configured window to be appended to TSDB.

This Pr addresses: #6167.

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

Added an experimental flag to thanos receive called --tsdb.too-far-in-future.time-window:

1. By default it is disabled with "0s"
2. If some values are provided, it will reject samples with timestamp t if t > now + window

Verification

Added two unit tests to verify it is working as expected

--- PASS: TestWriter (10.25s)
    --- PASS: TestWriter/should_error_out_when_sample_timestamp_is_too_far_in_the_future (1.02s)
    --- PASS: TestWriter/should_error_out_on_valid_series_with_out_of_order_exemplars (1.03s)
    --- PASS: TestWriter/should_error_out_when_exemplar_label_length_exceeds_the_limit (1.02s)
    --- PASS: TestWriter/should_error_out_and_skip_series_with_out-of-order_labels;_accept_series_with_valid_labels (1.02s)
    --- PASS: TestWriter/should_succeed_when_sample_timestamp_is_NOT_too_far_in_the_future (1.02s)
    --- PASS: TestWriter/should_error_out_and_skip_series_with_out-of-order_labels (1.02s)
    --- PASS: TestWriter/should_error_out_and_skip_series_with_duplicate_labels (1.02s)
    --- PASS: TestWriter/should_succeed_on_valid_series_with_exemplars (1.02s)
    --- PASS: TestWriter/should_error_out_on_series_with_no_labels (1.02s)
    --- PASS: TestWriter/should_succeed_on_series_with_valid_labels (1.03s)
PASS
ok      github.com/thanos-io/thanos/pkg/receive 10.899s

[hczhu]

Should check r.opts.TooFarInFutureTimeWindow != 0?

[davidyuanfs]

+1

[jnyi]

Good catch

[hczhu]

Maybe print an error and exit if the flag < 0.

[jnyi]

The value of the flag should be a duration string like "0s", "5m", "1h", etc

[davidyuanfs]

add debug log for how long it out of bounds?

[MichaHoffmann]

Could we even make it Warning level? That is something that should fail a bit louder i think so errors can be investigated better.

[davidyuanfs]

+1

[davidyuanfs]

is there a limit range we should allow it to put?

[jnyi]

ditto.

[fpetkovski]

What's the reason for making this flag hidden?

[jnyi]

no particular reason, i thought it is a convention for experimental flag, removed.

[GiedriusS]

I am a bit wary about adding yet another flag. Perhaps there could be some default value like 1h or 30m? What sense does it make to accept data in the future by default? 🤔 I wonder what @fpetkovski thinks about this PR.

[fpetkovski]

Adding a default of 1h sounds good to me. We can think about a flag or a more dynamic approach if the default proves to be not good enough 👍

[jnyi]

Sounds fair, let me make it default to 1h for now instead of a flag.

[matej-g]

I had same concern as @fpetkovski and @GiedriusS, for such a specific feature we should be fine with one value. I'd even go as far as saying whether it makes sense to accept anything with future timestamps, ideally users should not have any clock skew.

[defreng]

I kind of agree with what you're saying... I also don't see the point of allowing any future samples except for some minor clock skew which should be less than a few seconds typically (?)

If going with a static value, 1h seems far too big to me, and I would rather propose something like 30s or even less?

[ahurtaud]

We are currently looking at something to push points to the future. In order to build "seasonal" traffic, we are epxlorign a way to remote_write "current" throughput to +1 week timestamp. This way, we should be able to display / alert on the "1 week ago" traffic using "live" timestamp and not offset 1w. We expect this to be way more efficient, as we have performance issue with offset 1w. I am not sure it will be better with this yet. Will let you know...

EDIT: this would also allow us to display expected traffic like what we have in our in-house monitoring solution right now:

But we would like a flag to allow pushing to the future :/

[matej-g]

@ahurtaud interesting! Partly I was thinking if there is some special use cases where people want to push into future, and seems like you found one. I think it's reasonable then to add simple flag to enable / disable ingestion of future timestamps.

So how does it sound if:

1. By default we don't allow timestamps that are > 30 seconds in the future
2. This protection can be turned off with a flag

[ahurtaud]

perfect @matej-g ! thank you for considering it!

[jnyi]

That's good to know too @ahurtaud, I was about to revert the flag :( so in this case @matej-g I would argue this flag should be turned off by default for backward compatibility, people can turn on intentionally going forward by adding the flag --tsdb.too-far-in-future.time-window=<your perfered threshold>, our team has been adding the flag for a while internally for a week or so and it worked out well.

[matej-g]

@jnyi I don't have a strong opinion on the default, for me it would make sense to have the protection on by default, as the use case is so narrow and special, I believe 99% of users would be unaffected even if they did not know about this change.

[jnyi]

@jnyi I don't have a strong opinion on the default, for me it would make sense to have the protection on by default, as the use case is so narrow and special, I believe 99% of users would be unaffected even if they did not know about this change.

Agreed, I think also most people didn't even run into this issue so far so making it default doesn't matter to them neither, disable this by default is backward compatibility for people like @ahurtaud won't be affected when they upgrade Thanos, and people like us will be able to fix it as new adopters :)

[fpetkovski]

Thanks for the PR, this is looking quite good. I have mostly minor comments.

[fpetkovski]

What's the reason for making this flag hidden?

[fpetkovski]

Thanks, just one last comment from my side.

[fpetkovski]

Sorry if my comment wasn't clear, I meant that this appender should always reject samples that are too far in the future.

If the flag is set to a value other than zero, we would wrap the default appender with this one. Otherwise we would just use the storage appender as before.

[jnyi]

ic, we can do that, but I feel this pattern is more extensible for wrapping prometheus' default appender implementation going forward:

app := multiTSDB.appender(tenantID)
if tooFarInFuture != 0 {
  app = ReceiveAppender{ toofarInFuture, app}
} else if <another condition> {
 app = AnotherAppenderWrapper{ flag, app}
} else if <third condition> {
  app = thridAppenderWrapper...
}

vs

app :=  ReceiveAppender{
  tooFarInFuture: tooFarInFutureFlag,
  // add other flags
  app: multiTSDB.appender(tenantID)
}

// add behavior insider `ReceiveAppender`

[fpetkovski]

The appender wrapper would only be responsible for rejecting samples in the future and other appender wrappers will apply different limits. So it would rather be:

app := multiTSDB.appender(tenantID)
if tooFarInFuture != 0 {
  app = AppenderWithFutureLimit(toofarInFuture, app}
} 
if <another condition> {
 app = AnotherAppender{flag, app}
} 
if <third condition> {
  app = ThirdAppender(flag, appender)
}

In any case, this is definitely not blocker but more of a suggestion. We would anyway have one appender for now and we can break it up into multiple if it the logic gets more complicated in the future.

[fpetkovski]

Thanks, this looks good to me 👍

[JayChanggithub]

@jnyi @fpetkovski
I am not clearly but just wanna to clarify issue. I supposed these PR was validate whether cover my issue(#6158). If so how to add exactly parameters can solved with receiver? I found our
receiver instance query data not stability since promethues logs show msg="non-recoverable error" count=500 exemplarCount=0 err="server returned HTTP status 409 Conflict: 5 errors: forwarding request to endpoint thanos-receive-0.thanos-receive.thanos.svc.cluster.local:10901: rpc error: code = AlreadyExists desc = store locally for endpoint thanos-receive-0.thanos-receive.thanos.svc.cluster.local:10901: add 96 samples: too old sample; forwarding request to endpoint thanos-receive-1.thanos-receive.thanos.svc.cluster.local:10901: rpc error: code = AlreadyExists desc = store locally for endpoint thanos-receive-1.thanos-receive.thanos.svc.cluster.local:10901: add 103 samples: too old sample;
Thanks.

[jnyi]

Thanks, this looks good to me 👍

Thanks @fpetkovski , the e2e tests seem flaky, I try to trigger the rerun and dismissed your approval, would you take another stamp?

[yeya24]

Thanks!

[jnyi]

Hi @JayChanggithub,

The issue you mentioned seems a different one due to data samples too old, you could consider this flag instead --tsdb.out-of-order.time-window=1h

[JayChanggithub]

Hi @jnyi
Thanks for your replying. Also we had been adopted flag which called --tsdb.out-of-order.time-window=1h Unfortunately we still meet error code from each prometheus logs( msg="non-recoverable error" count=500 exemplarCount=0 err="server returned HTTP status 409 Conflict). It's impacted our receiver component to scrape each prometheus data as well as the promethues instances counts not stability. Anything else miss configuration regarding those ?

$ k get po -n thanos -oyaml thanos-receive-4  | kubectl-neat 

apiVersion: v1
kind: Pod
metadata:
  annotations:
    cni.projectcalico.org/containerID: 1924efd6089e38f2bc07c62a16f9b29a4fbd972caf3d10ba50922ddb974ceac4
    cni.projectcalico.org/podIP: 100.96.0.234/32
    cni.projectcalico.org/podIPs: 100.96.0.234/32
    kubernetes.io/psp: gardener.privileged
  labels:
    controller-revision-hash: thanos-receive-6484b9b744
    kubernetes.io/name: thanos-receive
    statefulset.kubernetes.io/pod-name: thanos-receive-4
    thanos-store-api: "true"
  name: thanos-receive-4
  namespace: thanos
spec:
  containers:
  - args:
    - receive
    - --grpc-address=0.0.0.0:10901
    - --http-address=0.0.0.0:10902
    - --remote-write.address=0.0.0.0:19291
    - --receive.replication-factor=1
    - --receive.hashrings-file-refresh-interval=1m
    - --receive.hashrings-algorithm=ketama
    - --objstore.config-file=/etc/thanos/objectstorage.yaml
    - --tsdb.path=/var/thanos/receive
    - --tsdb.retention=12h
    - --tsdb.out-of-order.time-window=1h
    - --receive.tenant-header="THANOS-TENANT"
    - --receive-forward-timeout=120s
    - --label=receive_replica="$(NAME)"
    - --label=receive="true"
    - --tsdb.no-lockfile
    - --receive.hashrings-file=/etc/thanos/thanos-receive-hashrings.json
    - --receive.local-endpoint=$(NAME).thanos-receive.thanos.svc.cluster.local:10901
  ......
  ..... 

[ranryl]

+1

[jnyi]

Hi @JayChanggithub @ranryl,

Your issue seems derail from what this PR is for. I don't think it is a good place to continue this discussion. Instead I would suggest you either open another github Issue or you can reach out to CNCF #thanos in #slack for help

[Avigdorrr]

Can we make this flag stable?
The flag have been merged over a year ago and i cant find any opened (or even closed) issues related to problems or bugs with it.

[GiedriusS]

Yes, help wanted. I think we can definitely mark it as stable.