Skip to content
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

Merged
merged 6 commits into from
Nov 4, 2020
Merged

Conversation

arlyon
Copy link
Contributor

@arlyon arlyon commented Oct 6, 2020

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

Copy link
Collaborator

@tomcoldrick-ct tomcoldrick-ct left a 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.

pkg/fetch/metrics_fetcher.go Outdated Show resolved Hide resolved
pkg/fetch/metrics_fetcher.go Outdated Show resolved Hide resolved
pkg/fetch/metrics_fetcher.go Outdated Show resolved Hide resolved
pkg/storage/asset_store.go Show resolved Hide resolved
Copy link
Collaborator

@tomcoldrick-ct tomcoldrick-ct left a 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!

@tomcoldrick-ct tomcoldrick-ct merged commit cd4deee into buildbarn:master Nov 4, 2020
@arlyon arlyon mentioned this pull request Nov 4, 2020
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