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

Properly read new percentile agg format #6309

Merged
merged 5 commits into from
Feb 25, 2016

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Feb 23, 2016

Built on top of #6308, which is required for the tests to pass.

Sometime recently the percentile aggregation's response format is changed from:

{
  "values": {
    "0.0": 123,
    "99.0": 456
  }
}

to

{
  "values": [
    { "key": 0, "value": 123 },
    { "key": 99, "value": 456 }
  ]
}

@spalger spalger force-pushed the fix/readingPercentileAgg branch from 03d8ac9 to a4afa37 Compare February 23, 2016 09:25
@spalger spalger assigned epixa and unassigned w33ble Feb 24, 2016
}) / 100;
const values = bucket[agg.parentId] && bucket[agg.parentId].values;
const percentile = _.find(values, value => agg.key === value.key);
return percentile ? percentile.value / 100 : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

The existing behavior would always return a number, can you think of any circumstances where returning null would cause issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't, but I suppose NaN is as good a default as any in this scenario.

@epixa
Copy link
Contributor

epixa commented Feb 24, 2016

I'm hopping on a plane, so I can't really dive into this any further, but I get a fatal error when I try to create a bar chart with a percentile ranks agg.

Using makelogs data and these settings:
screen shot 2016-02-24 at 10 06 50 am

I get the following fatal error:
screen shot 2016-02-24 at 10 15 35 am

The error didn't appear right away, either. Sometimes it would happen almost immediately, and sometimes it would take upwards of 10 seconds to appear (on crappy internet though).

@epixa epixa assigned spalger and unassigned epixa Feb 24, 2016
@spalger
Copy link
Contributor Author

spalger commented Feb 24, 2016

Confirmed that #6228 fixes the fatal error issue.

@spalger spalger assigned epixa and unassigned spalger Feb 24, 2016
@epixa
Copy link
Contributor

epixa commented Feb 25, 2016

We should get tests for some of this, but since this bug is currently blocking green builds on valid PRs, let's get this through.

LGTM

epixa added a commit that referenced this pull request Feb 25, 2016
Properly read new percentile agg format
@epixa epixa merged commit 41a6fb0 into elastic:master Feb 25, 2016
@spalger spalger deleted the fix/readingPercentileAgg branch February 25, 2016 22:50
@tbragin
Copy link
Contributor

tbragin commented Mar 25, 2016

Looks like the format change was a but in Elasticsearch and was recently fixed: elastic/elasticsearch#17217

Do we still need this change in v5.0.0 then?

@spalger
Copy link
Contributor Author

spalger commented Mar 25, 2016

@tbragin that bug was about the default format, we want to use the new format because it gives us actual numbers, not strings that we parse into numbers

cnasikas added a commit that referenced this pull request Apr 14, 2020
* Test utils

* Test get_configure

* Test post_configure

* Test get_connectors

* Test patch_configure

* Improve test

* Fixes

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
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.

4 participants