Skip to content
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

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Add track_timestamps_staleness #6317

merged 1 commit into from
Feb 6, 2024

Conversation

bboreham
Copy link
Contributor

@bboreham bboreham commented Feb 6, 2024

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

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • NA Config converters updated - this was done previously

Copy link
Member

@rfratto rfratto left a 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,
Copy link
Member

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
Copy link
Member

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.

@rfratto rfratto self-assigned this Feb 6, 2024
@bboreham
Copy link
Contributor Author

bboreham commented Feb 6, 2024

If you want to set the default to true

Yes I absolutely do, this is the correct setting for most people.

There was a lot of debate at #5972 (review)

@bboreham
Copy link
Contributor Author

bboreham commented Feb 6, 2024

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.

@rfratto
Copy link
Member

rfratto commented Feb 6, 2024

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

@rfratto rfratto merged commit d69623f into main Feb 6, 2024
10 checks passed
@rfratto rfratto deleted the timestamps-staleness branch February 6, 2024 17:48
Copy link
Contributor

@clayton-cornell clayton-cornell left a 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

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Feb 6, 2024
BarunKGP pushed a commit to BarunKGP/grafana-agent that referenced this pull request Feb 20, 2024
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Mar 8, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants