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

Input prometheus, use the same timestamp per call if no time is provided #7063

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

aarnaud
Copy link
Contributor

@aarnaud aarnaud commented Feb 21, 2020

avoid the chance to have different timestamp during iteration of each
metric for one http.Response

This PR, fix an issue we discover. some time during the iteration of metrics, the timestamp change to pass to the next second and our metrics aren't align.

This only applied if TimestampMs is not provided in the prometheus metrics

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@aarnaud aarnaud requested a review from danielnelson February 21, 2020 21:40
Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I have a suggestion on the comment that I think might clarify it a bit.

plugins/inputs/prometheus/parser.go Outdated Show resolved Hide resolved
plugins/inputs/prometheus/parser.go Outdated Show resolved Hide resolved
Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's take this a step further and call time.Now() only once and pass the time down into both makeQuantilesV2 and makeBucketsV2, this will give us full consistency for the query.

avoid the chance to have different timestamp during iteration of each
metric for one http.Response

Signed-off-by: Anthony ARNAUD <github@anthony-arnaud.fr>
@aarnaud aarnaud force-pushed the prometheus-metrics-align branch from 4da902f to 2c98cd0 Compare February 22, 2020 00:37
@aarnaud
Copy link
Contributor Author

aarnaud commented Feb 25, 2020

Do you now when the next release is plan ?

Can I made a PR to have a bugfix on 1.12 or 1.13 soon ?

@ssoroka
Copy link
Contributor

ssoroka commented Feb 25, 2020

As soon as it's merged we'll back port it to 1.13.x (probably 1.13.4). Just waiting for @danielnelson to approve.

@danielnelson danielnelson added area/prometheus fix pr to fix corresponding bug labels Feb 25, 2020
@danielnelson danielnelson added this to the 1.13.4 milestone Feb 25, 2020
@danielnelson danielnelson merged commit 8c99dc7 into influxdata:master Feb 25, 2020
danielnelson pushed a commit that referenced this pull request Feb 25, 2020
@aarnaud
Copy link
Contributor Author

aarnaud commented Feb 26, 2020

thanks @danielnelson for quick release. It's appreciate
I can't find the 1.13.4 release on dockerhub : https://hub.docker.com/_/telegraf?tab=tags
Is there a delay, or an PR a can make to trigger a build ?

@danielnelson
Copy link
Contributor

Sorry about this, currently it takes us a bit of time to get the official containers updated and unfortunately it isn't an automatic process. There are two places you can keep an eye on for progress updates:

athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prometheus fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants