feat(nice-grpc-prometheus): add metrics customization#423
feat(nice-grpc-prometheus): add metrics customization#423aikoven merged 2 commits intodeeplay-io:masterfrom
Conversation
aikoven
left a comment
aikoven
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
Overall it looks good, but please address the issues below 👇
| export const labelNames = [ | ||
| typeLabel, | ||
| serviceLabel, | ||
| methodLabel, | ||
| pathLabel, | ||
| codeLabel, | ||
| ]; |
There was a problem hiding this comment.
The set of labels is not the same for every metric. Particularly the code label is missing in some of them.
There was a problem hiding this comment.
I thought about it.
- There is indeed an extra label, but it looks like it has absolutely no effect on behavior.
- Attempting to separate labels leads to more complexity for the user and more possibility of mistakes.
An alternative way is to create custom wrappers for each metric that encapsulate the necessary parts. But this is too cumbersome.
What are your thoughts on this? Maybe there are some suggestions?
There was a problem hiding this comment.
Indeed it seems that putting a label to that list does not make it required and does not affect the metrics where we don't use it.
However according to the code of prom-client it would break if we use the metric.label(...) method: it checks that the array lengths are the same.
That said, since we don't use that method, I'm ok with having it this way.
There was a problem hiding this comment.
I've decided to separate labelNames and labelNamesWithCode for better reliability. Doesn't look as bad as I expected.
| import {Histogram} from 'prom-client'; | ||
|
|
||
| const clientHandlingSecondsMetric = new Histogram({ | ||
| registers: [registry], |
There was a problem hiding this comment.
The example here reuses the lib's registry, which means that you won't be able to use the same metric names. So to change the buckets you would have to use custom names, which does not sound convenient.
The tests use a separate registry, which I think is the correct way to go. But we need an example to mention this and also show how to merge the registries.
There was a problem hiding this comment.
You're right. It's updated now.
|
Looks great, thanks! I'm going to merge it and release shortly. |
|
Published 📦 |
2 participants
Hi!
I've found that the default buckets for histograms are not always suitable, and it would be useful to be able to customize them.
In looking for a way to change this I came to the conclusion that the most versatile and flexible way is to allow all metrics to be overridden.