Skip to content

Conversation

fitzoh
Copy link

@fitzoh fitzoh commented Mar 17, 2021

Expending a comment from #622 to a PR.

This PR proposes an API for recording exemplar values on counters and histograms.

Adds new <inc|add>WithExemplar(double value, String... exemplarLabels) methods which take string varargs containing exemplar values.

Inspired by initial golang implementation by @beorn7 : prometheus/client_golang#706

Not addressed

  • persisting exemplars/exposing them to scrapes (wanted to start with the validating the API)
  • histogram timer methods (adding exemplars to those could be tricky, doesn't need to block initial implementation)

Intentionally omitted

  • Counter#incWithExemplar(String... examplarLabels) increment by 1 convenience method (less duplication in the API, forcing the user to include an amount felt reasonable if they're already using the more verbose method).

@fstab @tomwilkie

Comment on lines +140 to +151
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial golang documentation that I can probably borrow from a little more heavily:
https://github.com/prometheus/client_golang/blob/c32ffd121f40843d8ce747536467bb091678fd87/prometheus/counter.go#L44-L51

There were two things I wanted to point out here.

Do we want to copy the go behavior of ignoring null labels?
I'm inclined to say that null labels are invalid and that the user should use inc instead.

Exceptions for invalid labels scare me a bit as well. Having instrumentation code throw an exception because your dynamically generated label ended up a little too long sounds like a not fun user experience, but I don't have a better alternative other than logging an error or incrementing an internal error counter.

fitzoh added 2 commits March 17, 2021 07:01
Signed-off-by: Andrew Fitzgerald <[email protected]>
Signed-off-by: Andrew Fitzgerald <[email protected]>
@fstab
Copy link
Member

fstab commented Mar 19, 2021

Thanks a lot for picking this up again. I continued the discussion in #622.

@fstab
Copy link
Member

fstab commented May 24, 2021

I just merged Exemplar support to the next-release branch, and it contains API very close to what you proposed here. Thanks for your proposal.

@fstab fstab closed this May 24, 2021
@fitzoh fitzoh deleted the exemplars branch May 24, 2021 21:23
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.

2 participants