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

Ability to override Created Timestamp on new metric or vector instance. #1364

Closed
bwplotka opened this issue Oct 10, 2023 · 5 comments · Fixed by #1375
Closed

Ability to override Created Timestamp on new metric or vector instance. #1364

bwplotka opened this issue Oct 10, 2023 · 5 comments · Fixed by #1375

Comments

@bwplotka
Copy link
Member

bwplotka commented Oct 10, 2023

Currently NewCounter (and similar for other metrics with created timestamp) and CounterVec.With.. family use time.Now() to obtain created timestamp.

This will work in majority of cases, but it will be a problem for exporters which might want to use those "normal" metrics (as opposed to collectors with e.g. const metrics) to report values they don't control (e.g cadvisor showing Linux counters). The counters they might expose can be started back in time, so created timestamp can't be "now" in the moment of exporter creating that metric.

We have to figure out API that would allow us to override time.Now() logic. This problem is a little bit related to #1354 but for production use case of modifying created timestamp. Some yolo ideas (didn't think about this through):

  • Don't change API, but update documentation that for those cases one should use NewConstMetricWithCreatedTimestamp. Alternatively ask in those cases to reset metric to 0 - both kind of bad UX.
  • Add to CounterOpts some CreatedTimestampFunc (prone to so many issues) or simply CreatedTimestamp time.Time. The latter is nice, but won't work with Vec easily 🤔
  • Add WithLabelValues...CT (and 3 others) that will allow specifying labels/label values and time.Time Created timestamp.
  • Invest in v2 API which will allow this if compatibility guarantee blocks us.

Acceptance Criteria:

  • Users have clear guidance (and possibility) to provide custom created timestamp to their counter based metrics.
  • Override has to be atomic (e.g. we can't just add "SetCreatedTimestamp" method to Counter as scrape can happen anytime between registration and `SetCreatedTimestamp).

cc @beorn7 @ArthurSens

@ArthurSens
Copy link
Member

Oh, nice use case, that didn't cross my mind.

I think it is important to not allow overriding the Now function in *Opts structs to avoid unexpected behavior with Prometheus

Without looking at how implementation would look like, I like the WithLabelValues().WithCT(time.Time) approach

@ArthurSens
Copy link
Member

Moving discussion from slack to this issue


@ArthurSens:
bwplotka, earlier today I was taking a look at #1364 and what interfaces would need to change to have WithLabelValues...CT(). For counter it seems pretty straight forward, but summaries and histograms's WithLabelValues returns an Observer interface. I'm still getting familiar with this... do you see any obvious problems when adding a new method like WithCreatedTimestamp(time.Time) Observer to that interface?

@ArthurSens:
One implementation that I don't completely understand is the ObserverFunc , not sure if it makes sense to have WithCreatedTimestamp to the ObserverFunc implementation 🤔

image

@bwplotka:
Actually I like your idea of using chained method. Lots of boilerplate code for us, but easy for user! I like it initially, let's try it out. Cc beorn

@bwplotka:
Ah I don't think this will work, as WithLabelValues or so give you final interface indeed, adding method with CT might be awkward

@bwplotka:
We could say no to custom CTs in normal APIs and ask users to do const metrics for it

@bwplotka:
Or collectors

@bwplotka:
Because in some way CT overriding is only needed for exported counters (counters that e.g. you mirror from Linux procfs counters). You wouldn't use Add and Inc in those scenarios either

@ArthurSens:
yeah makes sense.... It sounds like const metrics align better with real use cases

@ArthurSens
Copy link
Member

What documentation do we need to make clearer about NewConstMetricWithCreatedTimestamp?

@bwplotka
Copy link
Member Author

I would say we should put some created timestamp primer on our readme?

@ArthurSens
Copy link
Member

Maybe a test example so it shows up in godocs?

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

Successfully merging a pull request may close this issue.

2 participants