[TSVB] Introducing Timerange Data Mode for Metric Style Visualizations #15760

simianhacker · 2017-12-22T22:02:38Z

This PR attempts to fix a common usability issue with the metric style visualizations by adding a new setting that controls the time range used for matching documents. There are two options:

Entire time range will match all the documents selected in the time picker.
Last value will match only the documents for the specified interval from the end of the time range.

Before this setting users would often try to use the TSVB metric visualization to display a number for the entire time range not realizing that it was the last bucket. In some instances, users would add an "overall average" then scratch their heads wondering why the average would not match up to the raw values they calculated by hand. This PR is an attempt to try and alleviate that confusion by introducing this new concept up front.

This PR includes the following:

Adds time range data mode for table, gauge, metric and top n
Changes query for metric style vis to only query last 5 buckets unless
there is a sibling agg
Request and response are now included in api response
Fixed table header EUI upgrade
Fixed wrapping for top n

Last Value Mode

Entire Timerange Mode

When a user switches from "Last Value" mode to "Entire Timerange" mode and they have incompatible pipeline aggs

Merry Xmas @alexfrancoeur !

shaharmor · 2017-12-24T09:38:14Z

@simianhacker Not sure I understand this PR. Eventually the vis is showing only the last bucket, correct?
So all this PR adds is the ability to choose if to use the time picker as is, or only the last interval?
How will that help with the problem you described in the PR description? (Which, I have faced my self as well).

I have suggested it before, but I still think that given the open to remove the time dimension from those vis will be the better option, and make TSVB much more versatile

simianhacker · 2017-12-24T16:11:10Z

Removing the time dimension is not an option, the time picker is a global Kibana concept.

This should help because it puts the how we access the data front and center. If they choose last value then it uses the last value of the date histogram; this is very common if you are trying to display the current rate. If they choose entire time range then we disable the incompatible pipeline aggregations (and remove the date_histogram from the request) and display the value for the entire time range (which is the behavior of the core Kibana metric style visualizations).

A good example of this is if a user wants to use the filter ratio aggregation on the entire time range. Using "entire time range" mode will give them a different result then if they used the last value + and overall average.

shaharmor · 2017-12-24T16:13:39Z

and remove the date_histogram from the request

This is exactly what I meant by "remove the time dimension". This is awesome!
It will also mean that for calculating the sum over the entire range we won't have to use cumulative sum and thus make the table sortable, right?

simianhacker · 2017-12-24T16:26:40Z

Correct. Although the other table PR (#15747) makes all the fields sortable.

alexfrancoeur · 2017-12-27T15:53:32Z

@simianhacker any reason why we wouldn't allow this option for the time series visualization as well? Today we show a value for the last bucket in the legend. Ideally, we'd have similar logic for the legend value shown.

shaharmor · 2017-12-27T17:06:43Z

@alexfrancoeur the legend value shows the value that you "hover" on in the series, which is not the last bucket...
Do you mean what to show when you don't hover?

alexfrancoeur · 2017-12-28T16:12:01Z

@shaharmor you're correct, it does show the hover value. But without interaction, it shows a single metric which could be interpreted as the last full/partial bucket or overall value initially. Other than introducing tooltips, I don't see a good approach given the current UI. Let's see wait and see if this is ever requested in the future.

simianhacker · 2018-01-08T19:08:06Z

@alexfrancoeur The reason we don't have this feature in the "Time Series" visualization is because in "entire time range" mode it would just show single flat lines because it removes the date histogram all together (which isn't really that necessary in the metric type visualizations) but in a time series visualization it's critical. If the issue with the legends is the numbers then I think maybe we should just remove the numbers from the legend all together.

The sort order for the legend is based on the entire time range already. See #14679

simianhacker · 2018-01-17T17:59:25Z

Jenkins test this

weltenwort

This definitely improves a confusing aspect of the metric type visualizations. I suspect, however, that the interaction between the new "last value" interval and the "interval" and "drop last bucket" configuration in the panel options will still lead to confusion. From the code it looks like the "last value" interval defaults to the interval in the panel options, but the latter is ignored when the former is explicitly set.

Similarly, the effects of the "Override Index Pattern" options of the individual series are very difficult to correctly anticipate for the user.

How about combining the timerange mode controls and the "panel options" into one UI that only shows those inputs that are relevant under the given conditions? I don't have a clear idea how that could be done for the per-series overrides though.

Another way to help the user would be to display a summary of the currently applied settings somewhere, e.g. a text saying something like "results will be based on the last ${interval} interval before ${date}" or "results will be based on ${interval} intervals between ${from} and ${to}". Expressing that via the input controls would probably be preferable, though. Maybe a more literal interface like with the EUI "Expression" controls could help with that.

simianhacker · 2018-01-17T19:47:24Z

@weltenwort Great feedback. Let me see if I can come up with something a little more straight forward to clear up the confusion between the two intervals.

weltenwort

While I haven't finished going through the code I am seeing some unusual results in comparison to master:

"Last 30 days" on master:

"Last 30 days" on this branch:

It's probably related to the query changes made to support the new timerange mode.

weltenwort

Yay, the removal of the interval overrides makes it less confusing 👍

I did not manage to go through all of the code yet, but I left a first batch of comments below.

weltenwort · 2018-01-25T10:44:31Z

src/core_plugins/metrics/public/components/aggs/agg_select.js


 let enablePipelines = siblings.some(s => !!metricAggs.find(m => m.value === s.type));
 if (siblings.length <= 1) enablePipelines = false;

 let options;
 if (panelType === 'metrics') {
 options = metricAggs;
+ } else if (metricTypes.includes(panel.type) && panel.timerange_mode === 'all') {


This expression is repeated several times. How about creating a shared predicate with an expressive name, e.g. isMetric(panelType: PanelType): boolean?

weltenwort · 2018-01-25T10:48:03Z

src/core_plugins/metrics/public/components/aggs/agg_select.js

@@ -171,7 +181,8 @@ AggSelect.propTypes = {
 onChange: PropTypes.func,
 panelType: PropTypes.string,
 siblings: PropTypes.array,
- value: PropTypes.string
+ value: PropTypes.string,
+ panel: PropTypes.object


Passing both panelType and panel seems redundant, especially since the panel is only needed to gain additional access to panel.timerange_mode. Maybe something like timerangeMode: PropTypes.oneOf(['all', 'last']) would suffice? That would reduce the component's coupling to the internal structure of the (implicit) Panel type.

weltenwort · 2018-01-25T10:56:16Z

src/core_plugins/metrics/public/components/aggs/agg_select.js

@@ -129,14 +130,23 @@ function filterByPanelType(panelType) {
 }

 function AggSelect(props) {
- const { siblings, panelType } = props;
+ const { siblings, panelType, panel } = props;

 let enablePipelines = siblings.some(s => !!metricAggs.find(m => m.value === s.type));
 if (siblings.length <= 1) enablePipelines = false;

 let options;
 if (panelType === 'metrics') {


How can this ever be true? Isn't panelType === panel.type, which is one of ['timeseries', 'metric', 'gauge', ...]? Maybe this condition can be collapsed with the next if branch?

weltenwort · 2018-01-25T10:59:40Z

src/core_plugins/metrics/public/components/aggs/agg_select.js

+ ...pipelineAggs.filter(filterByPanelType(panelType))
+ .filter(agg => agg.value === 'calculation')
+ .map(agg => ({ ...agg, disabled: !enablePipelines })),
+ ];


There seems to be a lot of overlap with the next if branch. And there seem to be two similar mechanisms at work to disable certain aggregations: .filter()ing and using enabledPipelines. How about unifying this to one mechanism to reduce the number of "special cases" that need to be handled?

weltenwort · 2018-01-25T11:06:32Z

src/core_plugins/metrics/common/metric_types.js

+ 'metric',
+ 'top_n'
+];
+


As mentioned below, a predicate like isMetric(panelType: PanelType): boolean could go well with this and make conditionals more readable.

weltenwort · 2018-01-25T11:12:24Z

src/core_plugins/metrics/public/components/code.js

+ return (
+ <EuiTextColor color="accent">
+ <EuiCode>{children}</EuiCode>
+ </EuiTextColor>


It might be worthwhile to stick to the code styling provided by EUI? Maybe the design team is open to changing the code color in general if you feel it should be more pronounced.

@elastic/kibana-design ^

This went back and forth. It actually started with that accent color, but feedback was that it was too heavy so we went with the neutral github style.

I'd recommend sticking with the defaults and not overwriting it. Then if we do change it, the change would carry down.

cc @elastic/eui-design who will laugh at this one!

weltenwort · 2018-01-25T11:16:27Z

src/core_plugins/metrics/public/components/modal_confirm.js

+ EUI_MODAL_CONFIRM_BUTTON as MODAL_CONFIRM_BUTTON
+} from '@elastic/eui';
+export function ModalConfirm({
+ title = 'Are you sure?',


The EUI writing guidelines suggest to Avoid using "Are you sure" and labeling the buttons with their action, e.g. Switch and Keep.

This is just the component. What's better then Are you sure? for a default confirmation? I'm not going to change Yes and No for the defaults; when using this component, future developers can override the defaults with better syntax (as I did when I used it).

Yes, I can't think of better defaults myself TBH. Maybe not having default values at all in a generic component might be appropriate?

OR put something like title='TITLE GOES HERE' and then just leave the buttons as Yes and No; that should force people to change it. I don't like it when components can't be used out of the box (functionally); it makes the developer experience crappy. I would rather it work but say something obvious that I would just need to tweak then throw an error.

@simianhacker what if you use prop-types and make it a required prop?

@mattapperson We could do that too, let's be honest though... it's probably only ever going to be used in this instance. We've probably already talked too much about this, the end results has the correct text and this component only exists because I wrote it using KUI knowing that eventually I would have to swap it out with EUI and I just wanted to do that in one place.

weltenwort · 2018-01-25T11:20:22Z

src/core_plugins/metrics/public/components/lib/check_timeseries_pipelines.js

+ 'series_agg'
+];
+
+export function checkMetric(metric) {


How about calling this isValidTimeseriesMetric?

weltenwort · 2018-01-25T11:22:39Z

src/core_plugins/metrics/public/components/lib/check_timeseries_pipelines.js

+ );
+}
+
+export function checkTimeseriesPipelines(series) {


What do you think about using hasPipelineAggregation as a name here?

weltenwort · 2018-01-25T11:24:48Z

src/core_plugins/metrics/public/components/vis_editor.js

+ compatible with some of the metrics you have configured. If you
+ proceed the incompatible metrics will be removed.
+ </p>
+ <p>Are you sure you want to do this?</p>


Avoiding Are you sure? is recommended by the EUI writing guide (see earlier comment).

… at the end

simianhacker · 2018-10-03T00:29:30Z

Sorry for the rebase but it was the easiest way to revive this PR from the dead.

elasticmachine · 2018-10-03T01:14:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…aders

elasticmachine · 2018-10-03T14:53:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cchaos · 2018-10-18T13:08:48Z

Hey @simianhacker, I am working in converting TSVB's less to sass which also ends up touching a lot of js files for class renaming. I was wondering when you think this PR (and the couple other TSVB PR's out there) might go in? I would rather wait to submit mine after yours goes in so you won't have to worry about the rebasing.

elasticmachine · 2018-10-19T21:17:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-14T10:30:21Z

Pinging @elastic/kibana-app

elasticmachine · 2019-02-14T10:31:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

markov00 · 2019-02-14T10:32:53Z

@sulemanof this PR was specified here #15974 (comment) to solve te ordering issue on topN, could you take a look? I know that it's a bit outdated, but I don't think the internals of TSVB changes so much since then

simianhacker · 2019-05-20T16:11:18Z

Closing this PR due to lack of attention.

alexfrancoeur · 2019-05-20T16:27:02Z

😢

@AlonaNadler @timroes @stacey-gammon I realize this isn't as much of a focus these days, but it might be worth revisiting some day.

simianhacker added Feature:TSVB TSVB (Time Series Visual Builder) Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.2.0 v7.0.0 labels Dec 22, 2017

simianhacker added the review label Jan 16, 2018

simianhacker requested review from mattapperson and weltenwort January 16, 2018 23:10

simianhacker force-pushed the tsvb-data-timerange branch from 6d418b4 to 490798a Compare January 16, 2018 23:50

elastic deleted a comment from mattapperson Jan 17, 2018

weltenwort reviewed Jan 17, 2018

View reviewed changes

timroes mentioned this pull request Jan 24, 2018

Implement new inspector #16185

Closed

6 tasks

weltenwort requested changes Jan 25, 2018

View reviewed changes

weltenwort reviewed Jan 25, 2018

View reviewed changes

jimgoodwin added v6.2.1 v6.2.2 and removed v6.2.0 v6.2.1 labels Feb 6, 2018

simianhacker mentioned this pull request Feb 9, 2018

Several bugs in visual builder Top N after 6.x update #15974

Closed

jimgoodwin added v6.2.3 and removed v6.2.2 labels Feb 20, 2018

simianhacker added 10 commits October 2, 2018 17:05

Changed interval to be disabled instead of hidden

b105dfc

Use isMetric to determine if panel is metric type

62ce472

Adding test for drop_last_bucket

0dcf65d

Use isMetric instead of includs on the metricTypes array

39381d3

Clean up code by removing redundent components

5bb9374

Removing panel attribute

9d7384e

Fixing partial bucket support; adding offset so we have whole buckets…

51e3a59

… at the end

Fixing unit test

2123553

updating values based on the new bucketing scheme

46091ff

removing timezone from date histogram

16450f5

simianhacker force-pushed the tsvb-data-timerange branch from d96ced3 to 16450f5 Compare October 3, 2018 00:26

Fixing vis_with_splits hoc; fixing z-index on color picker; adding he…

e5e3cca

…aders

markov00 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Feb 14, 2019

snide added v7.0.1 and removed v7.0.0 labels Apr 10, 2019

simianhacker closed this May 20, 2019

rayafratkina mentioned this pull request May 22, 2019

Visual Builder interval auto for Top N, Metric and Gauge #23467

Closed

gospodarsky mentioned this pull request May 29, 2019

[TSVB] Introducing Timerange Data Mode for Metric Style Visualizations #37185

Merged

simianhacker deleted the tsvb-data-timerange branch April 17, 2024 14:51

[TSVB] Introducing Timerange Data Mode for Metric Style Visualizations #15760

[TSVB] Introducing Timerange Data Mode for Metric Style Visualizations #15760

Conversation

simianhacker commented Dec 22, 2017 • edited Loading

shaharmor commented Dec 24, 2017

simianhacker commented Dec 24, 2017

shaharmor commented Dec 24, 2017

simianhacker commented Dec 24, 2017 • edited Loading

alexfrancoeur commented Dec 27, 2017

shaharmor commented Dec 27, 2017

alexfrancoeur commented Dec 28, 2017

simianhacker commented Jan 8, 2018

simianhacker commented Jan 17, 2018

weltenwort left a comment

Choose a reason for hiding this comment

simianhacker commented Jan 17, 2018

weltenwort left a comment

Choose a reason for hiding this comment

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltenwort Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simianhacker Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simianhacker commented Oct 3, 2018

elasticmachine commented Oct 3, 2018

💔 Build Failed

elasticmachine commented Oct 3, 2018

💔 Build Failed

cchaos commented Oct 18, 2018

elasticmachine commented Oct 19, 2018

💔 Build Failed

elasticmachine commented Feb 14, 2019

elasticmachine commented Feb 14, 2019

💔 Build Failed

markov00 commented Feb 14, 2019

simianhacker commented May 20, 2019

alexfrancoeur commented May 20, 2019

simianhacker commented Dec 22, 2017 •

edited

Loading

simianhacker commented Dec 24, 2017 •

edited

Loading

weltenwort Jan 25, 2018 •

edited

Loading

simianhacker Mar 26, 2018 •

edited

Loading