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

Add telemetry for LRU cache #10079

Merged
merged 2 commits into from
Oct 13, 2020
Merged

Add telemetry for LRU cache #10079

merged 2 commits into from
Oct 13, 2020

Conversation

clly
Copy link
Contributor

@clly clly commented Oct 2, 2020

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.

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 2, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@mgritter mgritter left a 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.

sdk/physical/cache.go Outdated Show resolved Hide resolved
sdk/physical/cache.go Show resolved Hide resolved
@mgritter
Copy link
Contributor

mgritter commented Oct 7, 2020

These build failures:

vault/core_util.go:37:46: too many arguments in call to physical.NewTransactionalCache
	have (physical.Backend, int, hclog.Logger, *metricsutil.ClusterMetricSink)
	want (physical.Backend, int, hclog.Logger)

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.

Copy link
Contributor

@mgritter mgritter left a 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)..

@clly
Copy link
Contributor Author

clly commented Oct 9, 2020

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.

@clly
Copy link
Contributor Author

clly commented Oct 9, 2020

I ran go mod vendor and added what got put in there. One of the things that changed was the go.mod and go.sum for vault/api I wasn't 100% sure if it should get added but since it came from the command I figured yes?

I also fixed a metric -> metricSink change I missed and passed the correct MetricSink to the caches

@mgritter
Copy link
Contributor

mgritter commented Oct 9, 2020

I ran go mod vendor and added what got put in there. One of the things that changed was the go.mod and go.sum for vault/api I wasn't 100% sure if it should get added but since it came from the command I figured yes?

Yes, that's good.

I also fixed a metric -> metricSink change I missed and passed the correct MetricSink to the caches

Looks like cache_test.go needs to be fixed too, based on the latest CI run?

@clly
Copy link
Contributor Author

clly commented Oct 12, 2020

Makes sense to me. I forgot to update the tests after the latest change. I satisfied the requirement for a MetricSink by providing a BlackholeSink. If prefered I could attempt to mock the MetricSink interface to ensure that the expected methods are called.

Copy link
Contributor

@mgritter mgritter left a 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
@clly clly force-pushed the cache-telemetry branch 3 times, most recently from 07cdb1b to 485b384 Compare October 13, 2020 02:46
@clly
Copy link
Contributor Author

clly commented Oct 13, 2020

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.

@mgritter mgritter merged commit 3ad3c3d into hashicorp:master Oct 13, 2020
@mgritter
Copy link
Contributor

All set! I will add a Changelong entry for this.

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.

3 participants