Skip to content

implementation of metrics - #84

Merged
anatol merged 10 commits into
anatol:masterfrom
TheFox0x7:metrics
Jul 11, 2023
Merged

implementation of metrics#84
anatol merged 10 commits into
anatol:masterfrom
TheFox0x7:metrics

Conversation

@TheFox0x7

Copy link
Copy Markdown
Contributor

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

Comment thread pacoloco.go Outdated
@TheFox0x7

Copy link
Copy Markdown
Contributor Author

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.

Comment thread pacoloco.go Outdated
Comment thread pacoloco.go
Comment thread pacoloco.go Outdated
Comment thread pacoloco.go
Comment thread pacoloco.go Outdated
@anatol

anatol commented Jul 9, 2023

Copy link
Copy Markdown
Owner

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

Copy link
Copy Markdown
Contributor Author

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

@TheFox0x7

Copy link
Copy Markdown
Contributor Author

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

anatol commented Jul 9, 2023

Copy link
Copy Markdown
Owner

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

@TheFox0x7
TheFox0x7 marked this pull request as ready for review July 9, 2023 22:32
@TheFox0x7

Copy link
Copy Markdown
Contributor Author

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 anatol left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor naming issues, otherwise the change is good for the merge.

Comment thread purge.go Outdated
Comment thread pacoloco.go Outdated
Comment thread pacoloco.go Outdated
add some docs to stat gathering method
@anatol
anatol merged commit 9716b37 into anatol:master Jul 11, 2023
@anatol

anatol commented Jul 11, 2023

Copy link
Copy Markdown
Owner

Awesome, thank you for the PR!

@TheFox0x7

Copy link
Copy Markdown
Contributor Author

My pleasure :)

@anatol

anatol commented Jul 21, 2023

Copy link
Copy Markdown
Owner

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

Copy link
Copy Markdown
Contributor Author

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.

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