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

Added support to set an externally available timestamp to the metrics #494

Closed
wants to merge 1 commit into from

Conversation

cwiechmann
Copy link

@brian-brazil

I see this requirement has been discussed since a while and you wrote some time ago you would be happy to accept a PR to make it possible to take over already available timestamp information.

Some background about my use-case:
I can't instrument the code of an application directly as I don't own it, but the application provides me regular Metric-Information including a timestamp. The metric data is streamed to my Custom-Exporter almost in realtime, but there might be slight delays between the original event and when it is finally received by Prometheus.
That's the reason, why I would prefer to take over the given timestamp information into the exposed Metrics for Prometheus.

Signed-off-by: Chris Wiechmann <cwiechmann@axway.com>
@brian-brazil
Copy link
Contributor

Timestamps for direct instrumentation don't make sense, you want to use a custom collector which already support them.

@cwiechmann
Copy link
Author

Thanks @brian-brazil for the feedback.
When you say, I need to use a Custom-Collector, do you mean by that more or less the same thing as Counter, Gauge, Histogram, etc?
If yes, that would mean I need to re-implement exactly the same Collectors (duplicate the code) as the ones already existing just to make it possible to set existing timestamp. FYI: Before I created this PR, I already tried to overload the existing collectors, but that's not possible, as some of the fields are private.
If not, can you please give me a hint?

Thanks!

@brian-brazil
Copy link
Contributor

If yes, that would mean I need to re-implement exactly the same Collectors (duplicate the code) as the ones already existing just to make it possible to set existing timestamp.

When you're writing an exporter to consume push-based information, that's pretty normal.

@cwiechmann
Copy link
Author

Okay and understood.
However, I don’t get my head around to duplicate so much code just for that.

So, would you be okay to make it possible to overload the existing collectors?
I mean, I would create my own collectors based on the existing ones, which I will change hopefully slightly. For these changes I will create a new PR and let you know.

Just let me know and you may close this PR.

@brian-brazil
Copy link
Contributor

Being able to subclass these classes is not something I want to support, as that's not sane generally.

@cwiechmann
Copy link
Author

Hmm, I'm not fully sure, why it should harm to make the Client-Library a bit more flexible in one or the other way, as long as the standard use-cases are fully supported.
But you own it and you make the rules - So I'm going to close this PR. Thanks for your swift responses, much appreciated.

@cwiechmann cwiechmann closed this Aug 5, 2019
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.

None yet

2 participants