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

[TSVB] Add option to stack globally or within series #31417

Merged
merged 5 commits into from
Feb 26, 2019

Conversation

sulemanof
Copy link
Contributor

Fix #15543 .
The main idea is to change stack behavior in a plot depending on a particular series.
I suppose stack should only depends on the other data in the same series, and should not depend on others series (how it works currently).

The existing behavior is: =>

Configure 2 same series:
config1

Add a bucket script to the second metric to mirror results:
config2

The plot will look like: (1)
no-stack

Set stacked to negative metrics, the plot will look like: (2)
negative-stacked

Set stacked to positive metrics (in this case all series are stacked), the plot will look a bit frustrated: (3)
both-stacked-1

Furthermore, now it depends on a position of the first series. It means that if we change the position conversely, the plot will look like: (4)
both-stacked-2

Proposal solution:
The fix will not bring breaking changes into a plot (plots (1) and (2) will remain the same), but it will depends on a particular series now (both series are stacked, but do not depend on each other):
both-stacked

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sulemanof
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293 flash1293 self-requested a review February 19, 2019 16:03
@flash1293
Copy link
Contributor

It feels a bit dirty to add this check to the flot code itself. The documentation says

Two or more series are stacked when their "stack" attribute is set to the same
key (which can be any number or string or just "true"). 

This can be achieved by changing line 24 in src/legacy/core_plugins/metrics/server/lib/vis_data/helpers/get_default_decoration.js to
stack: series.stacked && series.stacked !== 'none' ? series.id : false,

However I'm not 100% sure whether this has some unintended side effects (pinging @timroes @ppisljar )

@sulemanof
Copy link
Contributor Author

sulemanof commented Feb 20, 2019

@flash1293 thanks for a review.
Agree with moving the logic inside get_default_decoration.js file, since the flot checks the stack for equality (so now it will check for same the series.id also).
But we have to decide is it an appropriate way to divide stack by particular series?

@timroes @markov00 please give us your vision

@flash1293
Copy link
Contributor

flash1293 commented Feb 20, 2019

@sulemanof and me looked into this via zoom and my proposed fix does the same thing as his, just in another place. The question remains whether this is the desired behaviour - if we add this, it won't be possible anymore to stack different series on top of each other. IMHO there is an argument for that because different series can be different metrics (e.g. average or count) and use different chart types (e.g. bar chart and line chart) which looks kind of strange and shouldn't be able to be stacked in the first place - kind of protecting the user from what they don't want to do in the first place and enabling them to do what the most probably wanted to do (stacking sub series for each series individually).
screenshot 2019-02-20 at 18 39 00

However this is a breaking change and maybe some users are depending on this. Another solution would be to make this behaviour configurable (a checkbox next to "Stacked" where you can enable "Only stack inside of the series").

EDIT: The more I think about this I think we should go with the configurable option. Any thoughts on this @timroes @markov00 @sulemanof ?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes
Copy link
Contributor

timroes commented Feb 21, 2019

Just talked to Joe about that behavior. I would second the suggestion that Joe made:

  • Let's add another option to the select called "Stack within series" (so we're having "None", "Stack", "Stack within series", "Percentage")
  • The value "Stack" would now keep stacking globally (including weird behavior in case you're stacking positive and negative values).
  • The value "Stack within series" would now apply that very behavior @sulemanof implemented here, that we are stacking within series.

It should be rather simple to implement that in get_default_decorationto set stack there to false in case the user selected none, true in case the user selected stack or percentageand the series.id in case it's set to stack within series.

Do you think that would make sense, @sulemanof ?

@sulemanof
Copy link
Contributor Author

Do you think that would make sense, @sulemanof ?

I suppose it makes sense! In such case it will be safety for existing visualizations!

@epixa epixa removed their request for review February 21, 2019 13:58
@flash1293
Copy link
Contributor

@sulemanof will you implement this directly on this PR or should we close this one and add a dedicated issue for the new feature?

@sulemanof
Copy link
Contributor Author

@sulemanof will you implement this directly on this PR or should we close this one and add a dedicated issue for the new feature?

I think it makes sense to implement it in the existing PR

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sulemanof
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Tested in Chrome Linux. Seems to work as expected. Code LGTM

@timroes timroes changed the title Add a check for the same series metric [TSVB] Add option to stack globally or within series Feb 26, 2019
@timroes timroes added release_note:enhancement Feature:TSVB TSVB (Time Series Visual Builder) labels Feb 26, 2019
@timroes timroes added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.2.0 labels Feb 26, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

Code LGTM

@sulemanof sulemanof merged commit 7559941 into elastic:master Feb 26, 2019
@sulemanof sulemanof deleted the #15543 branch February 26, 2019 15:23
sulemanof added a commit to sulemanof/kibana that referenced this pull request Feb 26, 2019
* Add a check for the same series metric

* Replace logic to the nodejs side

* Add 'Stack within series' option
sulemanof added a commit that referenced this pull request Feb 27, 2019
* Add a check for the same series metric

* Replace logic to the nodejs side

* Add 'Stack within series' option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:TSVB TSVB (Time Series Visual Builder) release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.2.0 v8.0.0
Projects
None yet
5 participants