-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 telemetry for LRU cache #10079
Add telemetry for LRU cache #10079
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new counters should be documented in vault/website/pages/docs/internals/telemetry.mdx as part of the same PR.
These build failures:
are probably the result of the changes in SDK not making it to the vendor directory (this is a difference between local builds and CI.) Try "go mod vendor" and see if CI is happier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we are good to go; you will need to fix the CI failures before committing (like I said above, this is probably because of changes in sdk)..
Thanks for the tip :) It seemed to build locally. But also I didn't realize Go doesn't use the vendor directory by default. I should have the fix in tonight. |
I ran I also fixed a metric -> metricSink change I missed and passed the correct MetricSink to the caches |
Yes, that's good.
Looks like cache_test.go needs to be fixed too, based on the latest CI run? |
Makes sense to me. I forgot to update the tests after the latest change. I satisfied the requirement for a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! You will need to merge in the master branch and resolve any conflicts, but if CI passes then you should be able to squash and merge (and if not, I can.) We're doing a feature freeze tomorrow so it'd be good to get these changes in so they can hit the next release.
Vault creates an LRU cache that is used when interacting with the physical backend. Add telemetry when the cache is hit, missed, written to and deleted from. Use the MetricSink from ClusterMetrics
07cdb1b
to
485b384
Compare
I ended up doing the rebase and squash first and then the merge. I tried a rebase on the branch instead which didn't do what I expected. I had to force push to try to fix up the changes, but I did the merge to master. I think it's right. If I messed it please let me know how but feel free to fixup the issues. Thank you so much for working through this with me. |
All set! I will add a Changelong entry for this. |
Vault creates an LRU cache that is used when interacting with the
physical backend. Add telemetry when the cache is hit, missed, written
to and deleted from.