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

Restore line rendering in point series vis using percentile agg #35649

Merged
merged 4 commits into from
May 20, 2019

Conversation

kertal
Copy link
Member

@kertal kertal commented Apr 26, 2019

Summary

When creating a visualisation using percentiles, no lines were rendered.

Fix for #34858

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@kertal kertal added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Visualizations Generic visualization features (in case no more specific feature label is available) bug Fixes for quality problems that affect the customer experience labels Apr 26, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@kertal kertal requested a review from markov00 April 26, 2019 12:47
@kertal kertal added the WIP Work in progress label Apr 26, 2019
@kertal kertal requested review from timroes and ppisljar April 26, 2019 12:47
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Tested locally seems to fix the missing line rendering problem.

Before merging I'd suggest to:

  • find the root cause of the problem: seems that what changes is not now the seriid is composed (with the dot) but the way it's configured on the seriConfig.data.id; I'd suggest to investigate where we the original change, substituting the dot notation of the seriConfig.data.id to only the initial percentage id.
  • if possible, try to add few tests to block any other possible changes of the seriId format for percentile aggregation

@kertal
Copy link
Member Author

kertal commented Apr 29, 2019

i took a closer look at it, this is a follow up problem of a refactoring, it's only relevant fo 7.x

the removal of aggId when building the series data obj

85bdd22#diff-0d98df44497e69dabf20a569326ad6caL31

This led to the following change:
85bdd22#diff-8363f944835538e447c3aedee84d3693L26

since there are composed ids for percentiles (1.5, 1.10, ...) this check didn't work anymore.

However, this can't be undone by adding aggId again, because the agg object is now longer available in this context

export function addToSiri(series, point, id, yLabel, yFormat, zFormat, zLabel) {

also it's not available in the context of the functions call

https://github.com/elastic/kibana/blob/2e232c2e31cdc9ebcf1c75201571f82be003ea2d/src/legacy/ui/public/agg_response/point_series/_get_series.js

solving it in a different way needs to be discussed with @ppisljar, it's not a small refactoring.

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code looks good,

  • we have an aggregation that produces multiple responses. each aggConfig has an Id. in response we id the columns with that same id col-aggid and if agg returns multiple responses we name the culmns col-aggId.subId
  • in editor we have a single configuration for all the responses agg produces (add percentile agg, go to metrics & axes tab and you will see there is a single series configuration.
  • vis gets the data in and processes it a bit (extracts the ids out of column ids for example)
  • later it tries to match series configurations to appropriate data series
  • as we have one config for all columns produced by one aggregation, we need to make sure matching works correctly

we could have this more explicit and have an option to provide multiple column ids for every serie config, or have duplicated series configurations for each of the columns, but i think this fix is fine for now, specially as we are looking to retire vislib in the long run.

@kertal can you please add a test for this ?

@kertal kertal force-pushed the pr-34858-percentile-dots-bug branch from c483693 to 30e4211 Compare May 16, 2019 17:39
@elasticmachine
Copy link
Contributor

💔 Build Failed

@kertal kertal requested a review from ppisljar May 20, 2019 09:12
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM, tested on chrome linux

@kertal kertal merged commit efe91c3 into elastic:master May 20, 2019
kertal added a commit to kertal/kibana that referenced this pull request May 20, 2019
…tic#35649)

Restores the visualization's  line rendering when using a multi metric aggregation like percentile. Before this fix only dots were rendered
fixes elastic#34858
kertal added a commit that referenced this pull request May 20, 2019
…) (#36684)

Restores the visualization's  line rendering when using a multi metric aggregation like percentile. Before this fix only dots were rendered
fixes #34858
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.2.0 v8.0.0 WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants