-
Notifications
You must be signed in to change notification settings - Fork 27
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
Prometheus metrics #174
Prometheus metrics #174
Conversation
Thanks for the PR! Will take a review next week. |
And it will be great if you can get the CI pass meanwhile. I have updated the toolchain version on master. |
src/extensions/prometheus/mod.rs
Outdated
Some(p) if p.is_empty() => None, | ||
p => p, | ||
}; | ||
let registry = Registry::new_custom(prefix, labels).expect("Can't happen"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why it can't fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only can fail if the prefix is equal to Some(p) where p.is_mepty()
and we take care of that in the match above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain it in the expect
? otherwise the next reader is likely going to have the same question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I think now it should be better
We are using your proxy for quite some time now and it works great for us, thank you for this :)
We needed metrics for our internal reporting & stuff so quite a while ago I have added them to our fork of subway. I also saw you have them marked as a TODO in the readme. So this pr is a work I have already done to introduce prometheus metrics to subway. If you think this would be nice to have here you go, of course Im happy to go through rounds of review :)
This pr contains 2 major changes.
To see changes related only to
jsonrpsee
bump look at the commitupdate jsonrpsee
and simiraly to see chanegs related only to prometheus inclusion take a look at theAdd prometheus metrics
Why jsonrpsee bump?
While it was not strictly need, without it the metrics related to the ws connection would be nearly impossible to write (and for sure it would be quite painfull to do).
Also since I did not have an access to your fork of the jsonrpsee this was the quickest thing I could do to make those changes.
What kind of metrics there are?
So there is ws/http counter metrics for open/closed sessions. Also metrics for the methods that are based on the ones that are used in the substrate - same names etc. Additionally metrics for cache misses per method.
Cc @xlc