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

Cleanup storage cache metrics #2897

Merged
merged 1 commit into from Aug 15, 2023
Merged

Conversation

SuperQ
Copy link
Contributor

@SuperQ SuperQ commented Apr 9, 2019

Split request and hit metrics into separate metrics, rather than using
labels. This avoids duplication of data and makes metric math easier.

  • Count cache errors separately to avoid weird math.
  • Hit ratio: registry_storage_cache_hits_total / registry_storage_cache_requests_total
  • Miss ratio: 1 - (registry_storage_cache_hits_total / registry_storage_cache_requests_total
  • Misses: registry_storage_cache_requests_total - registry_storage_cache_hits_total

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "cache_metrics" git@github.com:SuperQ/distribution.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #2897 into master will decrease coverage by 0.22%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2897      +/-   ##
==========================================
- Coverage   60.69%   60.46%   -0.23%     
==========================================
  Files         102      102              
  Lines        8044     8000      -44     
==========================================
- Hits         4882     4837      -45     
- Misses       2512     2516       +4     
+ Partials      650      647       -3
Flag Coverage Δ
#linux 60.46% <ø> (-0.23%) ⬇️
Impacted Files Coverage Δ
registry/storage/blobcachemetrics.go 53.84% <ø> (+1.99%) ⬆️
registry/client/blob_writer.go 45.23% <0%> (-8.26%) ⬇️
registry/client/repository.go 57.82% <0%> (-1.94%) ⬇️
registry/handlers/blobupload.go 44.72% <0%> (-1.09%) ⬇️
notifications/metrics.go 96.66% <0%> (-0.48%) ⬇️
registry/registry.go 32% <0%> (-0.29%) ⬇️
registry/client/auth/session.go 68.69% <0%> (+0.63%) ⬆️
registry/handlers/manifests.go 53.64% <0%> (+0.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14b96e5...e808fc5. Read the comment docs.

@SuperQ
Copy link
Contributor Author

SuperQ commented Sep 14, 2019

@manishtomar, can you also look at this one?

@milosgajdos
Copy link
Member

@SuperQ would you mind rebasing? Feel free to close if you no longer want to work on this.

@SuperQ
Copy link
Contributor Author

SuperQ commented Jul 14, 2023

@milosgajdos rebase done. Updated to also split out the errors into a separate metric. Things get weird when you have to figure out hits/misses/errors that way.

I wonder if it would be a good idea to migrate away from the docker/go-metrics wrapper package. It seems abandoned.

@milosgajdos
Copy link
Member

milosgajdos commented Jul 14, 2023

I wonder if it would be a good idea to migrate away from the docker/go-metrics wrapper package. It seems abandoned.

@SuperQ that's a good question. Normally I'd say why not, but do we have any extra requirements that are not met by that module? I dont think we do

@SuperQ
Copy link
Contributor Author

SuperQ commented Jul 14, 2023

I don't think the wrapper library adds much value.

Split request and hit metrics into separate metrics, rather than using
labels. This avoids duplication of data and makes metric math easier.

* Count cache errors separately to avoid weird math.
* Hit ratio: `registry_storage_cache_hits_total / registry_storage_cache_requests_total`
* Miss ratio: `1 - (registry_storage_cache_hits_total / registry_storage_cache_requests_total`
* Misses: `registry_storage_cache_requests_total -
registry_storage_cache_hits_total`

Signed-off-by: Ben Kochie <superq@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (807a836) 55.95% compared to head (c19adfd) 55.94%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2897      +/-   ##
==========================================
- Coverage   55.95%   55.94%   -0.01%     
==========================================
  Files         110      110              
  Lines       11050    11048       -2     
==========================================
- Hits         6183     6181       -2     
  Misses       4178     4178              
  Partials      689      689              
Files Changed Coverage Δ
...egistry/storage/cache/cachedblobdescriptorstore.go 58.13% <100.00%> (-1.87%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@deleteriousEffect deleteriousEffect left a comment

Choose a reason for hiding this comment

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

These look like solid changes to me 🚀

@milosgajdos milosgajdos merged commit ff46bf1 into distribution:main Aug 15, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants