Add exemplars to counter and histogram #706
Conversation
beorn7
force-pushed
the
beorn7/exemplars
branch
from
6af333e to
ce05c9f
Compare
|
Thanks, addressed both of the comments, but not yet the 64 character limit. Is that just the length of the label names and values added up, or does it include the curly braces and quotes and commas and perhaps even escaping? Is it in runes or in bytes? |
Just the names and values, before escaping.
Runes. Everything else in OM is about UTF-8 characters, so it'd seem odd to use bytes here. |
|
Thanks. Working on implementing the limit… |
beorn7
force-pushed
the
beorn7/exemplars
branch
from
ce05c9f to
ef972b9
Compare
|
Limit implemented. I assume the general direction is fine and will add tests now. |
beorn7
force-pushed
the
beorn7/exemplars
branch
2 times, most recently
from
719f3e9 to
502f6a5
Compare
|
This is now ready for review. |
prometheus/counter.go
Outdated
| // Labels will lead to a valid (label-less) exemplar. But if Labels is | ||
| // nil, the current exemplar is left in place. This method panics if the | ||
| // value is < 0, if any of the provided labels are invalid, or if the | ||
| // provided labels contain more than 64 runes in total. AddWithExemplar |
There was a problem hiding this comment.
Mightn't hurt to put numbers on this. What you and I consider costly for instrumentation isn't the same as everyone else, given that e.g. sending a udp packet on every event or using client-side quantiles is common.
There was a problem hiding this comment.
Hmm, those numbers would be quite involved. It's not just runtime, but also that the creation of the Exemplar needs a number of allocations.
Re-reading the current wording of the doc comment, I find it now a bit confusing myself. It would probably best to explain the usual strategy for really hot paths (stochastically do some observations with and most without exemplar or even do some more sophisticated adaptive rate limiting), but then it would get way too involved for a doc comment.
I removed the sentence about the performance impact for now. This is after all still an experiment. (I'll mark it as such in the changelog.)
There was a problem hiding this comment.
My thought was mainly to avoid scaring people away from using this for reasons that they don't actually care about. If they really care about a hot path, they'll probably have their own micro-benchmarks anyway.
prometheus/counter.go
Outdated
| @@ -78,6 +89,9 @@ type counter struct { | |||
| desc *Desc | |||
|
|
|||
| labelPairs []*dto.LabelPair | |||
| exemplar atomic.Value // *dto.Exemplar | |||
There was a problem hiding this comment.
Hmm, that looks weird as this comment is not in English but in Go.
To resolve the ambiguity here, I changed it to actual English: Always a *dto.Exemplar.
There was a problem hiding this comment.
Or more precisely: Containing nil or a *dto.Exemplar.
prometheus/promhttp/http.go
Outdated
| case PanicOnError: | ||
| panic(err) | ||
| case ContinueOnError: | ||
| // Handled later. |
There was a problem hiding this comment.
Returning here would make more sense now with the refactoring.
There was a problem hiding this comment.
I thought so, too, at first. But it would be redundant with the return in L191. I had it first there, and also had a default clause to explicitly handle all possibilities (which then doesn't require the final return in the function anymore), but then thought that's also redundant.
On the other hand, to really do the shortest possible code, the ContinueOnError could be completely omitted. I did that now, with a short comment.
Signed-off-by: beorn7 <[email protected]>
Signed-off-by: beorn7 <[email protected]>
beorn7
force-pushed
the
beorn7/exemplars
branch
from
502f6a5 to
c32ffd1
Compare
This still needs tests. I'd like to show it to you already, though, to find out if this is the right approach.
If it works for you, I'll add tests before merging this.