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

Allow to optionally specify timestamps #126

Open
lstrojny opened this issue Jan 14, 2023 · 6 comments
Open

Allow to optionally specify timestamps #126

lstrojny opened this issue Jan 14, 2023 · 6 comments

Comments

@lstrojny
Copy link

It would be terrific to be able to pass an optional chrono instance as a timestamp to specify the recency of the particular metric. My usecase is a prometheus exporter where the metrics are cached and it would therefore be correct to specify the time of the original retrieval of the metric instead of leaving out the timestamp which implicitly states "now".

@mxinden
Copy link
Member

mxinden commented Jan 18, 2023

Very much in favor of this. What do you think of adding this to the new ConstCounter and ConstGauge for now as neither of them would require synchronization and likely fits your use-case in an exporter? Contributions very welcome @lstrojny.

As an aside, as a developer of an exporter, I would greatly appreciate input on the new Collector trait: #82. You can see it in action on libp2p/rust-libp2p#3325.

@lstrojny
Copy link
Author

@mxinden adding it to Const* initially sounds good. I guess you would want to feature flag chrono support? I can try my luck with an implementation although be warned, I am doing more serious Rust for a week or so.

Will have a look at a custom collector trait but very likely only in a couple of days. From a short glimpse it could potentially change how I am driving metrics generation in my exporter.

@mxinden
Copy link
Member

mxinden commented Jan 18, 2023

Why is chrono needed? Given that we need unix timestamps only, wouldn't Instant and UNIX_EPOCH be enough?

@lstrojny
Copy link
Author

Good point, should indeed be enough

@lstrojny
Copy link
Author

One important note here: Prometheus requires millisecond timestamps while the OpenMetrics spec uses seconds. Since this is a Prometheus client not an OpenMetrics client it seems to be sensible to go with milliseconds but I wanted to bring it up.

@mxinden
Copy link
Member

mxinden commented Feb 27, 2023

One important note here: Prometheus requires millisecond timestamps while the OpenMetrics spec uses seconds. Since this is a Prometheus client not an OpenMetrics client it seems to be sensible to go with milliseconds but I wanted to bring it up.

My understanding is that the old Prometheus format expects ms, while newer versions of Prometheus will parse the timestamps as s when the content type is set to application/openmetrics-text; version=1.0.0; charset=utf-8.

@lstrojny can you confirm that this is / is not the case?

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

No branches or pull requests

2 participants