Skip to content

Conversation

@ryan-roemer
Copy link
Contributor

@ryan-roemer ryan-roemer commented Apr 25, 2017

For some background here -- we use node-cache daily in production and have some memory pressure issues that we want to make sure to rule out node-cache for / tune how we cache and what. One component of that analysis is that we log out the cache.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 sizeof library) to hard-code for

  • options.objectValueSize
  • options.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-cache is a Promise, so that we have first-class integration with dataloader. Thanks to the underlying clone library, 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 size 0 by this code: https://github.com/mpneuried/nodecache/blob/4.1.1/_src/lib/node_cache.coffee#L574-L576

else if _isObject( value )
  # if the data is an Object multiply each element with a defined default length
  @options.objectValueSize * _size( value )

Specifically the issue is:

_.size(new Promise(() => {})) === 0;

The PR here attempts to take a simplistic stab at this problem by providing a new options.promiseValueSize that is straight up used to produce sizes for stats, which no attempt at getting the "size" of the object. This avoids our _.size() is 0 problem.

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:

  • I'm using a then === 'function' type inference to detect "this value is a Promise, which I think is the most correct current way, but there are many others we could use.
  • We can't actually get the real underlying resolved data value without making _getValLength asynchronous. I figured that would be too big of change to just do unannounced 😉
  • I didn't add tests because the stats suite 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

@coveralls
Copy link

coveralls commented Apr 25, 2017

Coverage Status

Coverage increased (+0.002%) to 98.492% when pulling c1fd305 on FormidableLabs:feature/promise-stats into 24480d0 on mpneuried:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants