Skip to content
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 percentile ranks metric support #3470

Merged
merged 9 commits into from
Apr 3, 2015
Merged

Add percentile ranks metric support #3470

merged 9 commits into from
Apr 3, 2015

Conversation

drej82
Copy link
Contributor

@drej82 drej82 commented Mar 27, 2015

Added support for percentile rank metric to kibana. issue #3330
I've never used node.js before, so chosen approach might be wrong.

1. Rename percentList->valuesList
2. Add configurable boundaries to percent list control
Percentile ranks metric support
Not used after renaming it to values_list
@rashidkpc
Copy link
Contributor

it would appear that the email you've signed the CLA with does not match your github profile. Can you double check that so we can review this pull?

Also it appear the tests are failing

@drej82
Copy link
Contributor Author

drej82 commented Mar 30, 2015

Thanks, Rashid. I saw the test error, for some reason I didn't spot it on my box, maybe I didn't delete old file after renaming. I will fix it. CLA seems to be fine.

Update unit-test after renaming control
@drej82
Copy link
Contributor Author

drej82 commented Mar 31, 2015

Apparently, not all unit tests are run on my dev box. Maybe because it's Windows setup.

@spalger spalger self-assigned this Apr 2, 2015
var min = list[$scope.$index - 1] || 0;
var max = list[$scope.$index + 1] || 100;

var min = list[$scope.$index - 1] || $scope.valueBoundaries[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind moving the valueBoundaries argument into the attributes of the element where values-list is specified?

Something like:

<input type="text" value-list="some.property" value-list-min="0" value-list-max="100">

and

link: function ($scope, $el, attrs) {
  var min = _.parseInt(attrs.valueListMin);
  if (isNaN(min)) throw new TypeError('invalid value-list-min: ' + attrs.valueListMin);

  var max = _.parseInt(attrs.valueListMax);
  if (isNaN(max)) throw new TypeError('invalid value-list-max: ' + attrs.valueListMax);
}

@spalger
Copy link
Contributor

spalger commented Apr 2, 2015

Left some comments inline, but can you do me a favor and explain how you plan to use the percentile_rank metric? 😅

@drej82
Copy link
Contributor Author

drej82 commented Apr 2, 2015

@spalger,
RE: how you plan to use the percentile_rank metric

I want to have SLA in form - xx% request should complete within yy sec, to see whether my service satisfies SLA I want to use percentile_rank metric. Of course, I could create a lot of percentiles and see how many secs request takes on different %tiles, but I want to have one number to see how far the service from SLA. Does it make sense?

@spalger
Copy link
Contributor

spalger commented Apr 2, 2015

@drej82 sweet! that totally makes sense.

Here is an image of a percentile_rank agg showing that:

  • 3.675% of requests in the selected time range were less than 100 bytes
  • 9.479% were less than 1,000 bytes
  • 94.748% were less than 10,000 bytes
  • 100% were less than 100,000 bytes

image

@spalger
Copy link
Contributor

spalger commented Apr 3, 2015

LGTM! Thanks @drej82! 🍻

spalger added a commit that referenced this pull request Apr 3, 2015
Add percentile ranks metric support
@spalger spalger merged commit e4d45e6 into elastic:master Apr 3, 2015
@spalger spalger added the v4.1.0 label Apr 3, 2015
@drej82
Copy link
Contributor Author

drej82 commented Apr 3, 2015

Cool, thanks!

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

Successfully merging this pull request may close these issues.

3 participants