-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add Prometheus Metrics #12
Conversation
cd18ac9
to
89407c8
Compare
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.
Nice one, thanks a lot! Overall I think this patch looks great, just a few nits (plus responses to your comments and some todos).
I have written an impl for metrics_asset_store but haven't hooked it up yet. Will we need that? From what I can tell publishing asset store metrics is somewhat covered by the others.
In my humble opinion this is adequately covered from the blobaccess metrics along with the Fetch
and Push
server metrics.
I have published duration_seconds and size_bytes. I believe most other interesting things can be inferred. Is that thinking correct?
I think qualifiers may be an additional vector we might want, given their flexibility. From an operator perspective I'd probably like to know if, e.g. my git qualified fetches are failing more often or whatever. That said, such a thing is probably obvious anyway. Another potential thing that crossed my mind while looking at this is some kind of metric to allow me to calculate "% cache hit", however in the current design this is kinda awkward. I suspect that redesign related to #3 will be informative on this, so it's probably best to wait for that in any case.
a6c9507
to
190dc68
Compare
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.
Aside from the failing CI (looks like buildifier is not appeased), this looks good to me!
Recently bazel stopped bundling some dependencies that rules_docker depends on. This commit updates to the latest version.
190dc68
to
ddf1571
Compare
This PR publishes information about the pusher and fetcher to the prometheus metrics endpoint.
What is left out of this is publishing qualifiers, which may be an interesting metric, but will be included in a subsequent PR