Updated stats.js to delete counters #183

Merged
merged 8 commits into from Nov 29, 2012

Projects

None yet

3 participants

@patrickmccoy

Counters are now deleted after being used, not set to zero. This solves
some issues with high load on Graphite and certain graphing functions
which require null instead of 0 counters.

@mrtazz this is a patch for a conversation you had with @holybit a couple days ago on IRC. Please let me know if you want it done any differently, but the patch pretty much mirrors the change in the following gist: https://gist.github.com/3625157

@patrickmccoy patrickmccoy Updated stats.js to delete counters
Counters are now deleted after being used, not set to zero.  This solves
some issues with high load on Graphite and certain graphing functions
which require null instead of 0 counters.
01da23d
@mrtazz
Member
mrtazz commented Oct 27, 2012

Yeah the change looks ok, but apparently it breaks the build. Can you update the appropriate tests?

patrickmccoy added some commits Oct 30, 2012
@patrickmccoy patrickmccoy Update stats.js
Changed from delete to undefined, per some info on delete from Mozilla https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Operators/delete

This keeps the array element there, just sets it's value to undefined.
e3f7924
@patrickmccoy patrickmccoy Update test/graphite_tests.js
Updating the test to work with new null count change.
71df1bd
@patrickmccoy patrickmccoy Update stats.js
Added a config option delete_counters with a default: false to control the behavior of the counters metrics to delete.
777ea8c
@mrtazz
Member
mrtazz commented Nov 3, 2012

The tests are still failing on this one. Likely because we need a test with the option disabled and one with the option enabled. And then test for the different outcomes.

@jabbaugh

Any thoughts on when this might be merged?

@mrtazz mrtazz and 1 other commented on an outdated diff Nov 25, 2012
for (key in metrics.counters) {
- metrics.counters[key] = 0;
+ if (conf.deleteCounters) {
+ metrics.counters[key] = undefined;
@mrtazz
mrtazz Nov 25, 2012 Etsy, Inc. member

I think we should also change this to be delete(counters[key]); as it is in the gist. I don't really see the benefit of storing it as undefined and graphite will record None anyways it it doesn't receive a value afaik. So this is more or less just wasting memory. Or am I missing something here?

@patrickmccoy
patrickmccoy Nov 26, 2012

I think it is pretty much the same either way, but delete probably uses less memory. In the example below, both elements are undefined in the end, but setting a[2] = undefined stores undefined in the array, whereas delete doesn't, the javascript engine returns undefined when trying to access an element which has been deleted.

> a = ['a', 'b', 'c'];
[ 'a', 'b', 'c' ]
> delete(a[1])
true
> a
[ 'a', , 'c' ]
> a[1]
undefined
> a[2] = undefined
undefined
> a
[ 'a', , undefined ]
> a[2]
undefined

I can change this line to use delete if you think the memory savings will be significant.

@mrtazz
mrtazz Nov 26, 2012 Etsy, Inc. member

Depends on how you define "pretty much the same". Using delete we use less memory and don't have to loop over keys for which we aren't sending values anyways. So we are also saving some amount of CPU cycles. In the = undefined variant we just set the value to be the same as if the key didn't exist but have to store it and loop over the key. So I'd say let's go with the delete version.

@patrickmccoy
patrickmccoy Nov 26, 2012

Sounds good, I'll get make the change and add it to the pull by tomorrow.

patrickmccoy added some commits Nov 26, 2012
@patrickmccoy patrickmccoy Changed stats.js to use delete
Changed stats.js to use delete instead of setting the values to
undefined in order to save some memory.
c03f339
@patrickmccoy patrickmccoy Fixed the tests
Fixed the tests that were broken when changing to delete
d0901b8
@patrickmccoy

@mrtazz I have updated the code per our conversation last night to use delete() instead of setting the counters to undefined.

@mrtazz mrtazz merged commit d0901b8 into etsy:master Nov 29, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment