[TSVB] Introducing Timerange Data Mode for Metric Style Visualizations#15760
[TSVB] Introducing Timerange Data Mode for Metric Style Visualizations#15760simianhacker wants to merge 21 commits intoelastic:masterfrom
Conversation
|
@simianhacker Not sure I understand this PR. Eventually the vis is showing only the last bucket, correct? 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 |
|
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.
|
This is exactly what I meant by "remove the time dimension". This is awesome! |
|
Correct. Although the other table PR (#15747) makes all the fields sortable. |
|
@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. |
|
@alexfrancoeur the legend value shows the value that you "hover" on in the series, which is not the last bucket... |
|
@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. |
|
@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 |
6d418b4 to
490798a
Compare
|
Jenkins test this |
weltenwort
left a comment
There was a problem hiding this comment.
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.
|
@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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This expression is repeated several times. How about creating a shared predicate with an expressive name, e.g. isMetric(panelType: PanelType): boolean?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
As mentioned below, a predicate like isMetric(panelType: PanelType): boolean could go well with this and make conditionals more readable.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
The EUI writing guidelines suggest to Avoid using "Are you sure" and labeling the buttons with their action, e.g. Switch and Keep.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Yes, I can't think of better defaults myself TBH. Maybe not having default values at all in a generic component might be appropriate?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@simianhacker what if you use prop-types and make it a required prop?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
How about calling this isValidTimeseriesMetric?
There was a problem hiding this comment.
What do you think about using hasPipelineAggregation as a name here?
There was a problem hiding this comment.
Avoiding Are you sure? is recommended by the EUI writing guide (see earlier comment).
d96ced3 to
16450f5
Compare
|
Sorry for the rebase but it was the easiest way to revive this PR from the dead. |
💔 Build Failed |
💔 Build Failed |
|
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. |
💔 Build Failed |
|
Pinging @elastic/kibana-app |
💔 Build Failed |
|
@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 |
|
Closing this PR due to lack of attention. |
|
😢 @AlonaNadler @timroes @stacey-gammon I realize this isn't as much of a focus these days, but it might be worth revisiting some day. |
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 rangewill match all the documents selected in the time picker.Last valuewill 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:
there is a sibling agg
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 !