-
Notifications
You must be signed in to change notification settings - Fork 52
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 weightedPercentiles method to Histogram #31
Conversation
@felixge Any chance this would get merged eventually? This is just adding a new method on Histogram so there are no changes to any current behavior. |
Is this project no longer being actively maintained or is there an issue with this PR? |
It’s not being actively maintained. Who would be interested in helping out? |
I am interested, I am in the process of using this to enable codahal / dropwizard like library for node to enable easy metrics for SignalFx. |
I'm interested, too! |
@felixge I think we're both not actively using / maintaining this anymore. Can you give access to @csabapalfi and @fieldju and grant them push rights to the npm package? I can't access the project settings. |
@@ -61,6 +62,54 @@ Histogram.prototype.percentiles = function (percentiles) { | |||
return results; | |||
}; | |||
|
|||
Histogram.prototype.weightedPercentiles = function (percentiles) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this is not being included in toJSON(), which as far as I know is the main mechanism of getting data from a metric in this library.
I also see that this method does some heavy lifting and would have performance impacts if included in the JSON output.
How are you thinking people use this functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixge suggested a non-breaking change gets merged easier hence the new method. In my original PR I just replaced the percentiles method.
We were thinking of simply writing our own toJSON
using this new weightedPercentiles
method as we were not using everything from the original one anyway (e.g. mean/stddev that we didn't need for our non-normally distributed data e.g. latencies that we used this for)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out-loud
What if the constructor properties object of a Histogram was updated to in addition to taking the sample to use also took a call back that performed the logic required for the percentiles.
It could the default could be the current logic but you could choose to overwrite it.
const histogramExample = new Histogram({
percentileMethod: Histogram.wieghtedPercentiles
})
function Histogram(properties) {
properties = properties || {};
this._sample = properties.sample || new EDS();
this._percentileMethod = properties.percentileMethod || this.precentiles
this._min = null;
this._max = null;
this._count = 0;
this._sum = 0;
// These are for the Welford algorithm for calculating running variance
// without floating-point doom.
this._varianceM = 0;
this._varianceS = 0;
}
...
...
Histogram.prototype.toJSON = function () {
var percentiles = this._percentileMethod([0.5, 0.75, 0.95, 0.99, 0.999]);
return {
min : this._min,
max : this._max,
sum : this._sum,
variance : this._calculateVariance(),
mean : this._calculateMean(),
stddev : this._calculateStddev(),
count : this._count,
median : percentiles[0.5],
p75 : percentiles[0.75],
p95 : percentiles[0.95],
p99 : percentiles[0.99],
p999 : percentiles[0.999]
};
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually that makes perfect sense 👍 (Probably I also didn't bother since we used a custom toJSON
anyway but happy to make this change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@csabapalfi Can you update the following, so we can get this merged and released.
-
Update Histogram's constructor options to have a percentile function override, and make toJSON() use the override or default to the current logic.
-
Update README to reflect the new option
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, except for updating version in package.json
. I think it's better to use the command npm version minor
once the PR is merged so that as it'll auto-create a tag in git as well. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That command sounds fine to me, on my other OSS projects with Travis we do releases via the Github UI https://github.com/yaorg/node-measured/releases/new and that automatically creates the tag, triggers Travis and via https://github.com/Nike-Inc/cerberus-java-client/blob/master/.travis.yml#L9 causes the release to happen.
I think running that command and having a line similar to that will cause travis to release to NPM. I will merge this now and work on wiring this to Travis again under the new org. So don't make the tag yet.
Histogram now takes the result of priority sampling from ExponentiallyDecayingSample with taking the weights/priority into account.
See #27 for more details. (Essentially I'm trying to re-open #28)