Add options.promiseValueSize for promise values #84
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.
For some background here -- we use
node-cachedaily in production and have some memory pressure issues that we want to make sure to rule outnode-cachefor / tune how we cache and what. One component of that analysis is that we log out thecache.getStats()object to our Splunk logging and then run analysis on that.I'm currently instrumenting our code to get average sensible production values via deep object traversal size (using the
sizeoflibrary) to hard-code foroptions.objectValueSizeoptions.arrayValueSize... via offline analysis so I can get our live production
getStats()pretty accurate, and that's working fine except in one very specific scenario...The difficulty that we have encountered is that our biggest and most common value to store in
node-cacheis aPromise, so that we have first-class integration withdataloader. Thanks to the underlyingclonelibrary, this works well for the application, but the problem is that for our cache size analysis it's always incorrect because a promise is inferred as an object with size0by this code: https://github.com/mpneuried/nodecache/blob/4.1.1/_src/lib/node_cache.coffee#L574-L576Specifically the issue is:
The PR here attempts to take a simplistic stab at this problem by providing a new
options.promiseValueSizethat is straight up used to produce sizes for stats, which no attempt at getting the "size" of the object. This avoids our_.size()is0problem.I'm happy to rework this PR into any other form that would be workable for you in this project. And I will mention some caveats to my approach here:
then === 'function'type inference to detect "this value is aPromise, which I think is the most correct current way, but there are many others we could use._getValLengthasynchronous. I figured that would be too big of change to just do unannounced 😉statssuite appropriate, but looking to add new tests there seemed to muck with what you already have. Feel free to suggest a test path forward and I'll add coverage too...Thanks!
/cc @divmain @baer @ztsmith