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

Timestamps should be in seconds, eg in setToCurrentTime methods #167

Closed
kamalmarhubi opened this issue Dec 4, 2017 · 6 comments
Closed

Comments

@kamalmarhubi
Copy link

The implementation of setToCurrentTime for gauges uses the value of Date.now() which is in milliseconds. The suggested conventions for client libraries is to use base units for all recorded metrics.

@SimenB
Copy link
Collaborator

SimenB commented Dec 4, 2017

Very good point. Mind sending a PR for it? It's a breaking change, but that's fine. It will match the java client: https://github.com/prometheus/client_java/blob/1a8bf7da57402f444861c32abc9e0c7cd313748e/simpleclient/src/main/java/io/prometheus/client/Gauge.java#L178-L180

@siimon
Copy link
Owner

siimon commented Dec 5, 2017

Oh that one slipped under the radar! If you create a PR don't forget the change log :)

@kamalmarhubi
Copy link
Author

This can be done without breakage by adding a default-true flag legacyCurrentTimeMilliseconds on gauges. The flag can be set to false by people like me who want seconds. A breaking change can flip the default, and a further one can remove it.

I will probably PR this soon.

@siimon
Copy link
Owner

siimon commented Dec 7, 2017

I'm leaning towards making the breaking change right away instead of starting to add legacy properties, what do you think @SimenB ?

@SimenB
Copy link
Collaborator

SimenB commented Dec 7, 2017

+1 for just breaking. What we do now is wrong, I see no reason to keep supporting it

@kamalmarhubi
Copy link
Author

Happy to do either! :-)

ThomWright added a commit to ThomWright/prom-client that referenced this issue Feb 28, 2018
I think this is the only instance where this is incorrect.

Breaking change

Fixes siimon#167
siimon pushed a commit that referenced this issue Mar 9, 2018
* Use base units (seconds) for current time in gauge

I think this is the only instance where this is incorrect.

Breaking change

Fixes #167

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

No branches or pull requests

3 participants