search

anatol / pacoloco

Caching proxy server for Arch Linux pacman
MIT License
199 stars 30 forks source link

implementation of metrics #84

Closed TheFox0x7 closed 1 year ago

TheFox0x7 commented 1 year ago

This PR adds /metrics endpoint to pacoloco exposing some internal metrics.

TheFox0x7 commented 1 year ago

I've added reporting amount of mirrors per repo - if we took the example config it would look like this:

pacoloco_configured_mirrors_count{repo="archlinux"} 2 
pacoloco_configured_mirrors_count{repo="quarry"} 1
pacoloco_configured_mirrors_count{repo="sublime"} 1
pacoloco_configured_mirrors_count{repo="archlinux_reflector"} *

I'm considering adding upstream label for downloaded/aborted packages. So instead of looking at 2 aborted downloads 2 in archlinux repo if you queried for a non-existent package, you'd see 1 per upstream:

pacoloco_aborted_downloads_total{repo="archlinux",upstream="mirror.lty.me"} 1
pacoloco_aborted_downloads_total{repo="archlinux",upstream="mirrors.kernel.org"} 1

This would drop my only requirement for reporting amount of mirrors - they needed to be reported to accurately track aborts.

Let me know your thoughts on this.

anatol commented 1 year ago

Would it be possible to add a unit test for prometeus functionality like here https://stackoverflow.com/questions/63911468/unit-test-using-golang-prometheus-testutil ?

TheFox0x7 commented 1 year ago

It most likely would be. I'll take a look.

TheFox0x7 commented 1 year ago

I added metric tests to the currently written tests. I can decouple them if need be.

Cache size currently can only grow in size. The way I see it decreasing it would either require a separate timer to gather the size periodically or an extension of the current purge system but per repo so during the eviction I can subtract the deleted files from the currently reported value. Alternatively just count non-outdated ones as we will visit all of them anyway. PR #74 has some implementation of this. I could maybe integrate it into this patch. This would not handle manually deleted packages - it would false report them until restart or next purge routine (second implementation only), but I won't think it's an issue - especially if I'll go for the second version.

This would leave only prefetch to instrument.

anatol commented 1 year ago

The PR looks good to me. Thank you for your work!

TheFox0x7 commented 1 year ago

I've added the package size and count refresh on purge. It was way easier than I originally thought :)

I think this covers at least the basics for metrics. Turns out go is quite fun to learn blindly :)

anatol commented 1 year ago

Awesome, thank you for the PR!

TheFox0x7 commented 1 year ago

My pleasure :)

anatol commented 1 year ago

Hey @TheFox0x7 I refactored pacoloco download design. The downloader is faster and less error prone now.

If you have a chance - could you please check master for the prometeus functionality. The tests are passed, but having another pair of eye on the functionality will be useful.

TheFox0x7 commented 1 year ago

Looking at it should be matching the original implementation, testing manually on some packages everything worked as it should.

One minor unrelated thing I've found: downloader.go:104: downloading https://<mirror>/archlinux//extra/os/x86_64/ripgrep-13.0.0-3-x86_64.pkg.tar.zst. key has the Note the // after archlinux. same issue, though no one will notice. I Found both a root cause and fix for this and I will submit a PR for it in a few minutes.

Also thanks, you gave me a perfect excuse to tinker with my gitea instance and add a go CI.