fix(#13): Fix memory when systemStats are enabled #14
Conversation
- Fixed loop when systemStats are enabled - Added a non aggregated stats buffer to send this metrics - Functions that are exposed in the client object didn't change at all so it won't break anything - Added tests for new scenarios
|
@senzzzi can you check the failing tests and the conflicts please? |
There was a problem hiding this comment.
Left a few comments. There's some things I'd like to see changed mostly:
- Not mix test concerns between application and stats metrics
- Not use boolean argument flags (I know some of that code was already there)
- Remove logic/naming for aggregated stats metrics
Thanks for your work on this @senzzzi
spec/common.spec.js
Outdated
| // When | ||
| victim.aggregatedPut('my_metric1', 1, 'avg', 60); | ||
| victim.aggregatedPut('my_metric2', 1, 'avg', 60); | ||
|
|
||
| // Then | ||
| function onResponse(lines) { | ||
| httpsServer.stop(); | ||
| if (counter === 0) { |
There was a problem hiding this comment.
The tests are failing on this logic.
The order of the responses is not deterministic.
I think it's better and easier to split test concerns, as in test sending of application and system metrics in different tests.
There was a problem hiding this comment.
I get it, I will update it :)
| @@ -1,5 +1,7 @@ | |||
| 'use strict'; | |||
|
|
|||
| //jshint latedef: nofunc | |||
There was a problem hiding this comment.
What's the reason for this?
There was a problem hiding this comment.
We have a circular dependency on the functions we are using.. It does not allow the function declaration after being called.
lib/client.js
Outdated
| * @param aggregationFreq The aggregation frequency with which metric was aggregated. | ||
| * @param parameters An object with metric para meters: tags, agg, aggFreq, namespace and timestamp. | ||
| */ | ||
| function putMetric(self, metricTypeConf, name, value, aggregation, aggregationFreq, parameters, isStats) { |
There was a problem hiding this comment.
Although it is quite present in this codebase, I'm not a super fan of the 'boolean argument to change behaviour' pattern - maybe we can can create a new putStatsMetric instead?
See the putAggregatedMetric and putNonAggregatedMetric as an example of this.
System metrics are also never aggregated so I think we really need to treat them differently.
There was a problem hiding this comment.
I can change the behaviour if you want. I chose this way, because the function that the client exposes is strictly the same but we can call it saying it's a stats metrics without having to repeat code..
There was a problem hiding this comment.
Yes I'd prefer that. We can extract the common code to it's own function and re-use it on both functions.
lib/client.js
Outdated
| self.logger.debug(stringToLogHeader + stringToLog); | ||
| } | ||
| if (self.nonAggregatedStatsBuffer.bufferSize > 0) { | ||
| self.logger.debug('Flushing metrics (non aggregated): ' + self.nonAggregatedStatsBuffer.buffer); |
There was a problem hiding this comment.
Maybe change to 'Flushing stats metrics: ' + ... as stats metrics will never be aggregated.
lib/client.js
Outdated
| self.logger.debug('Flushing metrics (non aggregated): ' + self.nonAggregatedStatsBuffer.buffer); | ||
| } | ||
|
|
||
| //in case we need to send aggregated stats metrics, add code here |
There was a problem hiding this comment.
You can remove this comment since stats metrics are never going to be aggregated.
| } | ||
|
|
||
| /** | ||
| * Logs all the metrics to the logger |
lib/client.js
Outdated
| } | ||
| } | ||
|
|
||
| //in case we need to send aggregated stats metrics, add code here |
lib/client.js
Outdated
| * | ||
| * @param self A self client instance. | ||
| * @param metricLines The metrics, in valid line protocol, to push to the buffer. | ||
| * @param isMetricAggregated A boolean state if metrics are aggregated. |
There was a problem hiding this comment.
Stats metrics will never be aggregated.
lib/client.js
Outdated
| targetBuffer.buffer += metricLines; | ||
| targetBuffer.bufferSize++; | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe flush if the stats buffer is over capacity? It's probably not something that's going to happen frequently since the metric buffers will trigger a flush on all buffers much more often but the check isn't that expensive so.
There was a problem hiding this comment.
Well, that is exactly the reason we are doing this fix. When we call the flush function if the buffer is "full" (independent of the reason/threshold), it will call again the function to put the stats metrics in the buffer and so on and so on... I'd rather have another interval (smaller than the timeout to flush the other metrics) to flush the stats metrics.
lib/client.js
Outdated
| @@ -334,9 +476,17 @@ function putRaw(self, metric, value, parameters, isMetricAggregated) { | |||
| } | |||
|
|
|||
| if (!isMetricAggregated) { | |||
There was a problem hiding this comment.
This is why I'm really not a fan of the boolean argument flags.
The code becomes very unclear to follow, maybe you could refactor into different methods?
senzzzi
force-pushed
the
fix-systemstats-metrics
branch
from
dfa2dd1
to
33abace
Compare
so it won't break anything