-
Notifications
You must be signed in to change notification settings - Fork 486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add track_timestamps_staleness #6317
Conversation
Added upstream 4 months ago: prometheus/prometheus#13060
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a few comments about how to set it to true by default if that's what you think it should be.
Scheme: "http", | ||
HonorLabels: false, | ||
HonorTimestamps: true, | ||
TrackTimestampsStaleness: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bboreham If you want to set the default to true, this is the place to do it.
@@ -51,6 +51,7 @@ Name | Type | Description | Default | Required | |||
`enable_protobuf_negotiation` | `bool` | Whether to enable protobuf negotiation with the client. | `false` | no | |||
`honor_labels` | `bool` | Indicator whether the scraped metrics should remain unmodified. | `false` | no | |||
`honor_timestamps` | `bool` | Indicator whether the scraped timestamps should be respected. | `true` | no | |||
`track_timestamps_staleness` | `bool` | Indicator whether to track the staleness of the scraped timestamps. | `false` | no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bboreham If you want to change the default to true, this also needs to be updated.
Yes I absolutely do, this is the correct setting for most people. There was a lot of debate at #5972 (review) |
However it may be appropriate to merge this PR, which is just allowing the config, then do a separate PR where we start arguing again. |
Sure, if you think that's easier 👍 merging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs are fine as-is
Added upstream 4 months ago: prometheus/prometheus#13060 Co-authored-by: Paulin Todev <[email protected]>
PR Description
Added upstream 4 months ago: prometheus/prometheus#13060
Which issue(s) this PR fixes
#5921
Notes to the Reviewer
I have cherry-picked the commit originally in #5972; I did not yet change the default to true, because I'm unsure where that would go.
PR Checklist