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 timestamp support for ConstMetric #407

Closed

Conversation

simonpasquier
Copy link
Member

This PR addresses #187 since @brian-brazil mentioned that staleness handling is now ok.

Eager to hear from @beorn7 too. Maybe you had something else in mind regarding how to expose the timestamps in the client API.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@beorn7
Copy link
Member

beorn7 commented Apr 24, 2018

This looks doable to me. However, the ConstMetric creation is a bit clunky at the moment, and I am planning to redo it completely in v0.10. Thus, my grand plan was to introduce the timestamped metrics at that time, so that we don't introduce something now that we break six months later (or so... my scheduling is unfortunately determined by external factors).

On the other hand, if people really need it now (and possibly also without migrating to v0.10), we can do something like in this PR.

Let's see what others think.

@brian-brazil
Copy link
Contributor

brian-brazil commented Apr 24, 2018

OpenMetrics will also break this a bit, as we'll probably want to allow for more resolution than milliseconds here.

@beorn7
Copy link
Member

beorn7 commented Apr 25, 2018

I'm currently aiming for aligning the OpenMetrics adjustments with the v0.10 changes.

The questions here really is how much it is worth to have any possibility for timestamped metrics in v0.9 (which will then surely experience a breaking change when moving to the bright new v0.10/OpenMetrics universe).

@simonpasquier is this something you have concrete need for or did you just feel it's low hanging fruit to introduce?

@simonpasquier
Copy link
Member Author

I don't have an urgent need for it. I know of some internal projects at $work that would benefit from it but they are still at the prototype stage. Another candidate would be collectd_exporter but as collectd supports Prometheus natively since 5.7 and exposes the timestamps, this isn't a compelling case either.

Personally I'm fine with waiting for 0.10 unless other users need it now.

@beorn7
Copy link
Member

beorn7 commented Apr 25, 2018

OK, let's keep this PR open for a while to see if others chime in.

@luqasn
Copy link

luqasn commented Aug 9, 2018

We would really like to use this for some of our custom CloudWatch exporters, as the official cloudwatch-exporter now uses ConstMetric with timestamps and our own golang-exporters still have the timestamps offset, which makes them weird to work with.

@beorn7
Copy link
Member

beorn7 commented Aug 13, 2018

@luqasn OK, you got counted. :o)

Would it be possible for you to just merge this PR into your local fork of client_golang? If there were a handful of people requesting this, I'd consider it valuable to introduce a function now that we will break anyway in v0.10. If it's only you, it might be better in sum if you work from a slightly modified version until v0.10 is there.

@dkistner
Copy link

Would also like to have this merged :)

@beorn7
Copy link
Member

beorn7 commented Aug 15, 2018

OK, let's do it then. However, I have a better idea how to do it in detail. The current PR only handles the "plain" metrics, not histogram or summary. And I'd like to avoid the multiplicity of new functions. Stay tuned for a day or tow, and I'll present an alternative PR.

@brian-brazil
Copy link
Contributor

OpenMetrics is going for up to nanosecond resolution, accordingly I'd suggest using a time.Time.

@beorn7
Copy link
Member

beorn7 commented Aug 18, 2018

I have implemented my idea in #443. Please have a look everybody. I'll close this as superseded.

@beorn7 beorn7 closed this Aug 18, 2018
@jojimt
Copy link

jojimt commented Mar 26, 2019

@beorn7 With this change, it is possible to feed two samples of a series into a collector with two different timestamps. That's quite normal when the timestamp is attached by a hw or driver and there is some kind of buffering before the samples are collected. Is that supported? Or it will result in the "was collected before with the same name and label values" error?

@beorn7
Copy link
Member

beorn7 commented Mar 27, 2019

Generally speaking, this is against the scrape model of Prometheus. It might work by accident, but it might as well create an error.

It makes more sense to ask questions like this on the prometheus-users mailing list rather than in a (closed!) GitHub issue. On the mailing list, more people are available to potentially respond to your question, and the whole community can benefit from the answers provided.

@jojimt
Copy link

jojimt commented Mar 27, 2019

Thanks. I will take it to the mailing list (it was sort of related to the timestamp).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants