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.

Sign up for GitHub

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

Jump to bottom

[TSVB][Top N aggregation] Unable to deal with negative values #43581

Merged
merged 13 commits into from
Sep 6, 2019
Merged

[TSVB][Top N aggregation] Unable to deal with negative values #43581

merged 13 commits into from
Sep 6, 2019

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Aug 20, 2019 •
edited
Loading

Closes: #43579

Summary

Issue with the visual builder TopN Visualization. It is unable to deal with negative values correctly. The visualization seems to work correctly, if they use a Math script and display the absolute value.

image

Root cause analysis

This issue happens because TopN view always uses 0 as a min value for calculating line width. I think it's not right and instead of 0 we should use the min value based on showed data

image

What was changed in this Pull Request

For negative values instead of 0 I offer to add 1% offset based on min value. It should fix problem with negative numbers. See:
image

For positive values it should works like before

image

Checklist

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

For maintainers

@alexwizp alexwizp requested a review from a team August 20, 2019 13:21
@markov00
Copy link
Member

Hey @StephanBenning I think that one was first time of someone using the TopN with negative values (@AlonaNadler can you confirm this?). We'd like to know from you and Alona what do you imagine to be the right behaviour with negative values.

@alexwizp alexwizp added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Aug 20, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@alexwizp alexwizp added v7.4.0 v8.0.0 Feature:TSVB TSVB (Time Series Visual Builder) labels Aug 20, 2019
@alexwizp alexwizp self-assigned this Aug 20, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alexwizp
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@markov00
Copy link
Member

My preference on is to emulate a standard horizontal bar chart with negative values.
Usually the negative value on that chart is depicted with the 0 value in the left side of the chart and the bar that grows from the left to right like in this example the red bars:

Positive-Negative-Bar-Chart-9

Having the base line on the left for negative values maybe can leads to wrong assumption is the user didn't see the minus sign.

There is also a subsequent case: what happen when we have positive and negative numbers?

@alexwizp alexwizp added the enhancement New value added to drive a business result label Aug 22, 2019
@alexwizp
Copy link
Contributor Author

Thanks @markov00 I like an idea with green/red lines. Unfortunattly I haven't seen it before in Kibana.
@AlonaNadler @timroes Should we implement that behavior?

@AlonaNadler
Copy link

TopN is often using when in several use cases:

  • When users want to use TSVB functionality that doesn't exist in Visualize but not explicitly over time (playing with the interval size and not dropping the last bucket)
  • When users want to show the last bucket as appose to the entire time range on the time picker
    For the first questions, yes we should support negative values

I love @markov00 suggestions and think it is ideal if possible

@markusTh83
Copy link

@markov00 I like your idea of positive/negative bars. I've had a similar idea.
The colouring could be user specified, the topN visualization of the TSVB already contains that feature. Furthermore there could be some sort of checkbox, ie. ,,field contains negative values'' to enable or disable the feature of negative x-axis bars - disabled by default to not affect the actual behaviour.

@alexwizp
Copy link
Contributor Author

Thanks Team, so I've added 3 different modes:

  1. Only positive mode:
    image

  2. Only negative mode:
    image

  3. Mixed mode:
    image

Switching works automatically based on the transmitted data.

@AlonaNadler @markov00 could you please check it?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alexwizp
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alexwizp alexwizp requested review from TinaHeiligers and myasonik and removed request for myasonik September 2, 2019 12:28
@alexwizp alexwizp added v7.5.0 and removed v7.4.0 labels Sep 3, 2019
@TinaHeiligers
Copy link
Contributor

@alexwizp, @timroes and @markov00 are out. I will review from the Kibana-App team.

TinaHeiligers
TinaHeiligers reviewed Sep 3, 2019
View reviewed changes
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

I ran this in Chrome and it works as intended with negative values, keeping the 0 line in the center of the visualization when there are positive values too. The range for negative and positive values depends on the max of the individual scales, as previously requested but does not change when the data refreshes (the range remains the same). I'm not sure if the range is intended to be dynamic.
Example: I create the visualization:
TSVB_TopN_with_Neg_Values

Once the data refreshes, I get:
Screen Shot 2019-09-03 at 3 19 30 PM
If it's intended to not change the range (scale of the graph), then it LGTM.

@alexwizp
Copy link
Contributor Author

alexwizp commented Sep 4, 2019

@TinaHeiligers I see 2 different values (-4.1KB and -1.5KB) on your charts it's a reason of different views

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Sep 4, 2019 •
edited
Loading

@alexwizp the different values are because the data refreshed. What I was trying to ask about is why does the range of the visualization not change?

TL;DR: does the range of the visualization get calculated on each render?

For example, in the first case, we have the most negative value of -4.1KB and the range of the negative values is -10KB to 0KB.
After a the refresh interval, the data refreshes, and we get a most negative value of -1.5KB, the range of the negative values is still -10KB to 0KB. Does the range not change as the data changes?
What if we now get a value of -13KB, will the bar for that be cut off or will the range extend past -10KB?

@alexwizp
Copy link
Contributor Author

alexwizp commented Sep 5, 2019

@TinaHeiligers we recalculate range on each render. In general formula for calculating range (valid only for MIXED mode) looks like:

range = Math.max(Math.abs(minValue), maxValue)

Then we use the range value as max value for positive and negative part of chart. As you see we use one value cause it's important to save proportions between all data in chart.

About your example: why you see -10KB. interval. It's just because max positive value is 10KB

@alexwizp alexwizp dismissed markov00’s stale review September 5, 2019 07:52

Marco is not available

TinaHeiligers
TinaHeiligers approved these changes Sep 5, 2019
View reviewed changes
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Testes in Chrome and it works as expected.
LGTM.

@alexwizp alexwizp merged commit bf9c86f into elastic:master Sep 6, 2019
alexwizp added a commit to alexwizp/kibana that referenced this pull request Sep 6, 2019
@alexwizp
[TSVB][Top N aggregation] Unable to deal with negative values (elasti…
e8dfab0 
…c#43581)

* [TSVB][Top N aggregation] Unable to deal with negative values

* change logic only for negative values

* fix PR comments

* fix scss styles

* fix calculating interval length

* Fix PR comments
@alexwizp alexwizp mentioned this pull request Sep 6, 2019
Merged
alexwizp added a commit that referenced this pull request Sep 6, 2019
@alexwizp
[TSVB][Top N aggregation] Unable to deal with negative values (#43581) (
812f6a2 
#44972)

* [TSVB][Top N aggregation] Unable to deal with negative values

* change logic only for negative values

* fix PR comments

* fix scss styles

* fix calculating interval length

* Fix PR comments
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 6, 2019
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into fix/autocompl…
9e128a9 
…ete-for-distance_feature

* 'master' of github.com:elastic/kibana: (89 commits)
  Replace TSVB timeseries charts with elastic-charts (elastic#33558)
  [TSVB][Top N aggregation] Unable to deal with negative values (elastic#43581)
  [alerting] Adds Action Type configuration support and whitelisting (elastic#44483)
  FTR: fix WebDriver Actions calls (elastic#44605)
  [Code] add NodeRepositoriesService to watch new repositories on local node (elastic#44677)
  [skip-ci][Maps] Improve Maps intro page (elastic#44721)
  [Maps] Update titles and descriptions for data sources (elastic#44833)
  Types + Extract Integration Util (elastic#44433)
  Downgrade log level from info to debug for cases when we cannot handle authentication attempt. (elastic#44933)
  [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable (elastic#44359)
  Update Jest script to output coverage (elastic#44447)
  [ftr] support --kibana-install-dir flag (elastic#44552)
  [WATCHER] Allow user to set a threshold value of 0 (elastic#44810)
  Remove injectI18n in dashboard plugin. (elastic#44580)
  [Graph] Save modal (elastic#44261)
  Use external script for the OIDC Implicit flow handler page. (elastic#44866)
  disable router prefixing with pluginId (elastic#44855)
  [SIEM] Fix bug on url + inspect functionality on hosts/hostDetails page (elastic#44671)
  [ML] File data viz limiting uploaded doc chunk size (elastic#44768)
  [code] Append go env variable 'GOCACHE' to go lsp spawn command. (elastic#44864)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 6, 2019
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into basic_sql_tem…
5d85255 
…plate

* 'master' of github.com:elastic/kibana: (91 commits)
  [APM] Make number of x ticks responsive to the plot width (elastic#44870)
  [ML] Single metric viewer: Fix top nav refresh behaviour. (elastic#44860)
  Replace TSVB timeseries charts with elastic-charts (elastic#33558)
  [TSVB][Top N aggregation] Unable to deal with negative values (elastic#43581)
  [alerting] Adds Action Type configuration support and whitelisting (elastic#44483)
  FTR: fix WebDriver Actions calls (elastic#44605)
  [Code] add NodeRepositoriesService to watch new repositories on local node (elastic#44677)
  [skip-ci][Maps] Improve Maps intro page (elastic#44721)
  [Maps] Update titles and descriptions for data sources (elastic#44833)
  Types + Extract Integration Util (elastic#44433)
  Downgrade log level from info to debug for cases when we cannot handle authentication attempt. (elastic#44933)
  [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable (elastic#44359)
  Update Jest script to output coverage (elastic#44447)
  [ftr] support --kibana-install-dir flag (elastic#44552)
  [WATCHER] Allow user to set a threshold value of 0 (elastic#44810)
  Remove injectI18n in dashboard plugin. (elastic#44580)
  [Graph] Save modal (elastic#44261)
  Use external script for the OIDC Implicit flow handler page. (elastic#44866)
  disable router prefixing with pluginId (elastic#44855)
  [SIEM] Fix bug on url + inspect functionality on hosts/hostDetails page (elastic#44671)
  ...
@alexwizp alexwizp deleted the tsvb_topN_negative branch January 4, 2020 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TinaHeiligers TinaHeiligers TinaHeiligers approved these changes

@timroes timroes Awaiting requested review from timroes

@AlonaNadler AlonaNadler Awaiting requested review from AlonaNadler

@markov00 markov00 Awaiting requested review from markov00

Assignees

@alexwizp alexwizp

Labels
enhancement New value added to drive a business result Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.5.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TSVB][Top N aggregation] Unable to deal with negative values
7 participants
@alexwizp @markov00 @elasticmachine @AlonaNadler @markusTh83 @TinaHeiligers @rayafratkina