Redis cache fixes and metrics #3113
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0b4063f to
18c1de0
Compare
manishtomar
reviewed
Mar 5, 2020
| func NewPrometheusCacheProvider(wrap cache.BlobDescriptorCacheProvider, name, help string) cache.BlobDescriptorCacheProvider { | ||
| return &prometheusCacheProvider{ | ||
| wrap, | ||
| // TODO: May want to have fine grained buckets since redis calls are generally <1ms and the default minimum bucket is 5ms. |
There was a problem hiding this comment.
This is something that is required. This will require update in go-metrics API to take in the buckets.
There was a problem hiding this comment.
Yeah, it would require exposing the prometheus opt through go-metrics. The default bucket is reasonable, this is a good TODO but not a requirement for this.
| latencyTimer metrics.LabeledTimer | ||
| } | ||
|
|
||
| func NewPrometheusCacheProvider(wrap cache.BlobDescriptorCacheProvider, name, help string) cache.BlobDescriptorCacheProvider { |
There was a problem hiding this comment.
I wonder if we should document all the metrics exposed by registry under docs/. Need not be this PR.
There was a problem hiding this comment.
That sounds like a good exercise in which I would be interested in the result
There was a problem hiding this comment.
A single /metrics output should be sufficient to start with. I've seen other projects do that.
Test base functionality of the cache statter Signed-off-by: Derek McGowan <derek@mcgstyle.net>
* fix redis caching issue earlier redis cache was updated when there was any error including any temporary connectivity issue. This would trigger set calls which would further increase load and possibly connectivity errors from redis leaving the system with continuous errors and high latency. Now the cache is updated only when it is genuine cache miss. Other errors do not trigger a cache update. * add back tracker Hit() and Miss() calls *squashed commits* (cherry picked from commit 6f3e1c10260ef59ba4e9c42e939329fad9fdd8c3) (cherry picked from commit 6738ff3320cf82cc2df919a95a1bde2f7789a501) Signed-off-by: Derek McGowan <derek@mcgstyle.net>
* redis metrics it is working but metrics are not very useful since default buckets start from 5ms and almost all of them are in that range. * remove extra comment (cherry picked from commit ba1a1d74e7eb047dd1056548ccf0695e8846782c) Signed-off-by: Derek McGowan <derek@mcgstyle.net>
* Update redis.go If the dgst key does not exist in the cache when calling HGET, `redis.String` will return an `ErrNil` which we need to translate into `distribution.ErrBlobUnknown` so that the error being returned can be properly handled. This will ensure that `SetDescriptor` is properly called from `cachedBlobStatter::Stat` for `repositoryScopedRedisBlobDescriptorService` which will update the redis cache and be considered as a Miss rather than an Error. cc @manishtomar * Update suite.go Add unit test to ensure missing blobs for scoped repo properly return ErrBlobUnknown when HGET returns redis.ErrNil. (cherry picked from commit dca6b9526a1d30dd218a9f321c4f84ecc4b5e62e) Signed-off-by: Derek McGowan <derek@mcgstyle.net>
Separate fix for cherry-picked code Signed-off-by: Derek McGowan <derek@mcgstyle.net>
18c1de0 to
d5acfc4
Compare
Closed
manishtomar
approved these changes
Mar 9, 2020
The metrics tracker in cached blob statter was replaced with prometheus metrics and no longer needed. Remove unused log wrapping. Signed-off-by: Derek McGowan <derek@mcgstyle.net>
d5acfc4 to
be29c05
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
add prometheus metrics for cache calls