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

[Lens] Support metric trendlines #141851

Merged
merged 69 commits into from
Oct 18, 2022
Merged

Conversation

drewdaemon
Copy link
Contributor

@drewdaemon drewdaemon commented Sep 26, 2022

Summary

resolve #135882

Screen.Recording.2022-10-10.at.2.04.21.PM.mov

Outstanding Issues

  • Trendline A11Y stuff
  • Check for loss of n/a in metric vis w/ no data
  • Turning off the trendline breaks layer
  • Always call onDrop with target
  • Functional tests
  • Move layer hiding to group config
  • Suggestions (can we detect when we're dealing with the default timefield?) — answer: no
  • Disable for dataviews with no time field (including dataview switch)
  • Check for remove dimension flyout closing case (can't reproduce but a test fails when I remove it)
  • Create reducers tech debt issue
  • remove trendline layer when chart type is switched (check if hidden?)
  • Interaction between max dimension and trendline switch
  • No feedback when enabling if there isn't enough data to render trendlines
  • Functional test for linked dimensions?
  • Trendline suggestions (waiting until next iteration)
  • Support collapse-by
  • Fix duplicate requests problem (will be resolved by Allow to disable partial results in expressions #143344)

Checklist

Delete any items that are not applicable to this PR.

Verified

This commit was signed with the committer’s verified signature.
kmuehlbauer Kai Mühlbauer
@drewdaemon drewdaemon removed the skip-ci label Oct 3, 2022
@drewdaemon drewdaemon added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Oct 4, 2022
@flash1293
Copy link
Contributor

@markov00 keep in mind that this always happens if there are null data points between existing data points which is a common situation to occur if the date histogram interval drops below the sampling rate of the source data.

@drewdaemon
Copy link
Contributor Author

@flash1293 this is ready for another look. I think we're getting close to ready for takeoff! 🚀

@flash1293
Copy link
Contributor

This looks almost good, there is just one thing I noticed that we should fix and I think it's easy to fix.
The secondary dimension is always synced, even if it's not referenced by the breakdown dimension and thus not required in the trendline layer.

This fails in the case of a reduced time range for the secondary metric:
Screenshot 2022-10-14 at 10 18 28

I think this behavior makes sense for the primary metric - doing a trendline and a reduced time range at the same time is kind of exclusive, but I think showing a reduced-time-range secondary metric is very helpful to combine with a trendline.

It seems like the syncColumns logic in the datasource is almost smart enough already to do this - what we could do there is to check whether a synced terms column is referencing a column not available in the target layer and sync it over too.

@drewdaemon
Copy link
Contributor Author

@KOTungseth I know you had some feedback on the PoC for this feature. Do you have any feedback on the copy here?

Mostly it's the supporting visualization button group and the messaging when one supporting visualization type is disabled.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and didn't find any problems anymore, LGTM once green. Awesome work!

Test failures seem simple to fix

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
expressionMetricVis 61 62 +1
lens 933 935 +2
total +3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
expressionMetricVis 56 62 +6
lens 568 575 +7
visualizations 725 730 +5
total +18

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
expressionMetricVis 73.1KB 73.5KB +432.0B
lens 1.3MB 1.3MB +11.1KB
visTypeGauge 7.1KB 7.1KB +19.0B
visualizations 268.2KB 268.2KB +16.0B
total +11.6KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
expressionMetricVis 0 1 +1
lens 45 46 +1
total +2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
expressionMetricVis 10.1KB 13.5KB +3.4KB
lens 29.1KB 29.2KB +168.0B
visualizations 53.4KB 53.7KB +299.0B
total +3.8KB
Unknown metric groups

API count

id before after diff
expressionMetricVis 56 62 +6
lens 659 668 +9
visualizations 755 760 +5
total +20

ESLint disabled line counts

id before after diff
lens 21 20 -1

References to deprecated APIs

id before after diff
maps 81 77 -4
ml 249 239 -10
total -14

Total ESLint disabled count

id before after diff
lens 24 23 -1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@drewdaemon drewdaemon merged commit 66041ca into elastic:main Oct 18, 2022
@kibanamachine kibanamachine added v8.6.0 backport:skip This commit does not require backporting labels Oct 18, 2022
kc13greiner pushed a commit to kc13greiner/kibana that referenced this pull request Oct 18, 2022
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Hey, @andrewctate! It looks like I've arrived late, as this PR is merged now. Nonetheless, I have a handful of comments and questions that I've written up some below for your review. Hopefully we can address them in a follow-up PR:

  • In our last vis editors sync, we discussed potentially moving "Supporting visualization" (and the conditional "Bar direction" button group) into its own section within the primary metric configuration flyout, below "Appearance." With a section heading of "Supporting visualization, we could then change the "Supporting visualization" label for the button group to something shorter, like "Type." Is that being split off as a separate issue? Or did we decide against it after our discussion?

  • Adding a reduced time range to the primary or secondary metric while "Trend line" is selected causes an error to appear in the workspace. Can we instead automatically fallback to using "None" for the supporting visualization to avoid the error altogether? This would also be more consistent with how we handle maximum value dimensions being removed when "Bar" is selected.

  • Changing the supporting visualization type to "Trend line" and closing the flyout causes the visualization layer's previous "Clear" button to incorrectly change to a "Delete" button. Attempting to then delete the layer via this button causes a number problems to occur. For the metric type visualization, the action should always be clear. Delete shouldn't be an option, as metric visualizations only support a single layer.

metric-bug

{
id: `${buttonIdPrefix}trendline`,
label: i18n.translate('xpack.lens.metric.supportingVisualization.trendline', {
defaultMessage: 'Trend line',
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that this text gets truncated at smaller viewport sizes. Would it make sense to change Trend line to simply Line to avoid the truncation?

isFullWidth
buttonSize="compressed"
legend={i18n.translate('xpack.lens.metric.progressDirectionLabel', {
defaultMessage: 'Bar direction',
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on changing this text from Bar direction to Bar orientation?

if (!state.maxAccessor) {
supportingVisHelpTexts.push(
i18n.translate('xpack.lens.metric.summportingVis.needMaxDimension', {
defaultMessage: 'Add a maximum dimension to enable the progress bar.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest changing this text to: Bar visualizations require a maximum value to be defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

To use bar charts, you must use a maximum value to enable the progress bar.

supportingVisHelpTexts.push(
!hasDefaultTimeField
? i18n.translate('xpack.lens.metric.supportingVis.needDefaultTimeField', {
defaultMessage: 'Use a data view with a default time field to enable trend lines.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest changing this text to: Line visualizations require use of a data view with a default time field.

Copy link
Contributor Author

@drewdaemon drewdaemon Oct 20, 2022

Choose a reason for hiding this comment

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

I thought that, since we aim to give the user direction, the imperative voice was most appropriate. Could you clarify your reasoning?

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis Oct 20, 2022

Choose a reason for hiding this comment

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

Ultimately, I'll defer to @KOTungseth for all of my wording suggestions here. However, my reasoning for suggesting this direction (and moving away from imperative) was to put the name of the visualization being referenced first in the sentence for easier scanning (since there exists the possibility of showing multiple help text sentences across multiple visualization types (bar, line).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll implement your suggestions in #143781 and @KOTungseth can chime in there next week if she has time 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

To create line charts, you must use a data view with a default time field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I worry that "To create line charts" implies greater flexibility than we offer with the metric trend lines (on/off switch). I wonder if even getting away from calling them trend lines is a good idea (getting back to Michael's original feedback on the button itself).

WDYT? I ultimately defer to you 👍

: metricHasReducedTimeRange
? i18n.translate('xpack.lens.metric.supportingVis.metricHasReducedTimeRange', {
defaultMessage:
'Remove the reduced time range on this dimension to enable trend lines.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest changing this text to: Line visualizations cannot be used when a reduced time range is applied to the primary metric.

: secondaryMetricHasReducedTimeRange
? i18n.translate('xpack.lens.metric.supportingVis.secondaryMetricHasReducedTimeRange', {
defaultMessage:
'Remove the reduced time range on the secondary metric dimension to enable trend lines.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest changing this text to: Line visualizations cannot be used when a reduced time range is applied to the secondary metric

@drewdaemon
Copy link
Contributor Author

Hi @MichaelMarcialis . I merged this PR because the functionality is complete and it was incurring a maintenance burden, but I'm not finished with the feature polishing. Thanks for your comments.

In our last vis editors sync, we discussed potentially moving "Supporting visualization" (and the conditional "Bar direction" button group) into its own section

This is still in the plans 👍

Adding a reduced time range to the primary or secondary metric while "Trend line" is selected causes an error to appear in the workspace. Can we instead automatically fallback to using "None" for the supporting visualization to avoid the error altogether? This would also be more consistent with how we handle maximum value dimensions being removed when "Bar" is selected.

I agree with you here. Falling back to "None" was my first choice. But, quite a bit of sleight of hand was required to deliver on the "layerless" trend line experience so the technical machinery isn't really comparable to the progress bar. Due to this, falling back to "None" (i.e. triggering the removal of the hidden trend line layer when a setting on a datasource operation changed) didn't look like a reasonable option from a technical perspective.

I was glad to get it at least to where the user clearly sees what action caused the problem and if they come at it in the other order, we handle it more gracefully with the disabled button and the help text. Happy to discuss further if you feel strongly.

Changing the supporting visualization type to "Trend line" and closing the flyout causes the visualization layer's previous "Clear" button to incorrectly change to a "Delete" button.

Excellent find. Here we see the true implementation leaking through. Created an issue here #143687

I'll wrap your copy suggestions up in another PR 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure ui-copy Review of UI copy with docs team is recommended v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Metric grid: Trend line support
9 participants