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

[Charts] Multi-layer time axis (opt-out only) #115853

Merged
merged 27 commits into from
Oct 27, 2021

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Oct 20, 2021

Summary

This PR introduce the multi-layer time axis in Discover, Lens, Visualize, TSVB.
It adds visualization:useLegacyTimeAxis advanced settings under charts plugin to toggle legacy time axis.

The new multi-layer time axis is introduced in @elastic/charts https://elastic.github.io/elastic-charts/?path=/story/area-chart--timeslip and was demoed as part of the Kibana Demo Days.

It is the outcome of the research done in elastic/elastic-charts#1310 related to improve the time axis solving the following problems:

  • sparse time labels that can be far apart
  • unclear where time point is on the label (the middle)
  • difficult / tedious to read due to redundant information and small fonts
  • resolution is not explicit (is it hours of days or days themselves)

Screenshot 2021-10-26 at 17 43 39

There can be some subsequent fix PR related to that, that DRY up the code a bit and introduce small changes in the colors.

TO REVIEWERS: this change can have some issues that we are promptly collecting here elastic/elastic-charts#1442 and fixing in the next weeks. If the case we can't fix all the issues before nov 15 we are going to switch on the visualization:useLegacyTimeAxis switch.

image

@nickofthyme nickofthyme changed the title [Charts] Add setting for using legacy time axis [Charts] Add setting for using legacy time axis [skip-ci] Oct 20, 2021
@nickofthyme nickofthyme added the WIP Work in progress label Oct 20, 2021
@nickofthyme nickofthyme requested a review from markov00 October 20, 2021 20:13
@markov00
Copy link
Member

@elasticmachine merge upstream

@elastic elastic deleted a comment from kibanamachine Oct 26, 2021
@nickofthyme nickofthyme added closed:enhancement Feature:ElasticCharts Issues related to the elastic-charts library Feature:Lens labels Oct 26, 2021
@markov00 markov00 marked this pull request as ready for review October 27, 2021 11:37
@markov00 markov00 requested review from a team as code owners October 27, 2021 11:37
@elasticmachine
Copy link
Contributor

Pinging @elastic/datavis (Feature:ElasticCharts)

@markov00
Copy link
Member

I've marked the PR ready to review because it looks like the CI is timing out on the ciGroup6 multiple times
https://buildkite.com/elastic/kibana-pull-request/builds/1985#
but this looks like an issue not related to the changes

@monfera monfera requested review from monfera and removed request for monfera October 27, 2021 12:25
@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] OSS CI Group #2 / embeddable explorer dashboard container pie charts

Metrics [docs]

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
charts 253 254 +1

Async chunks

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

id before after diff
discover 340.0KB 340.4KB +480.0B
lens 1.0MB 1.0MB +700.0B
visTypeTimeseries 485.3KB 485.8KB +504.0B
visTypeXy 61.8KB 62.2KB +452.0B
total +2.1KB

Page load bundle

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

id before after diff
charts 59.0KB 59.1KB +85.0B
discover 22.9KB 23.0KB +136.0B
kbnUiSharedDeps-npmDll 5.0MB 5.1MB +90.3KB
lens 39.2KB 39.4KB +136.0B
visTypeTimeseries 14.8KB 14.9KB +136.0B
visTypeXy 40.1KB 40.4KB +240.0B
total +91.0KB
Unknown metric groups

API count

id before after diff
charts 285 286 +1

History

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

@flash1293
Copy link
Contributor

Still waiting for bootstrap, but some notes:
For other settings like this we add the "deprecated" label to the advanced setting from the start to signal we want to get rid of it eventually:
Screenshot 2021-10-27 at 16 17 21

We should probably do this here as well.

Is there a reason timelion is excluded? This PR changes a test for some reason (maybe some change of behavior in legacy tick placement) but none of the code.

@markov00
Copy link
Member

For other settings like this we add the "deprecated" label to the advanced setting from the start to signal we want to get rid of it eventually

Sure we can add the deprecation label but we don't have a specific version yet where we like to remove this option.
@ghudgins ideas?

Is there a reason timelion is excluded? This PR changes a test for some reason (maybe some change of behavior in legacy tick placement) but none of the code.

The issue with timelion is that it doesn't have a great set of defaults, in combination with the multi-layer time axis can create confusion and misleading charts. One example is the size of the bars: timelion doesn't show bar histograms, but it uses small thin bars of 6px by default. The bars are rendered always between the starting point and the end point, but when the size is reduced so much, the bar looks centered in the middle of the bin and the user can misunderstand that the x value is in the middle, not at the beginning.
This also helps the phasing out of Timelion from kibana :)

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Dashboard snapshot testing tolerance change LGTM! Code review only.

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

I've logged few questions and bugs in the elastic-chart linked issue.

One bug is relative to Visualize normal bar chart not enabling the new time axis.
The other bug is relative to some offset in Vertical bar chart in Lens (probably with partial data?)

@flash1293
Copy link
Contributor

Sure we can add the deprecation label but we don't have a specific version yet where we like to remove this option.

We just changed the tooltip for pie chart to "in a future version" because it wasn't decided yet. I think we can do the same here.

@markov00 markov00 merged commit 6b4d9dd into elastic:master Oct 27, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 115853

@markov00 markov00 changed the title [Charts] Multi-layer time axis (opt-in only) [Charts] Multi-layer time axis (opt-out only) Oct 27, 2021
@markov00 markov00 removed the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 27, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 115853 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 29, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 115853 or prevent reminders by adding the backport:skip label.

@spalger spalger added the backport:skip This commit does not require backporting label Nov 1, 2021
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 1, 2021
@nickofthyme nickofthyme deleted the legacy-time-axis-setting branch November 1, 2021 23:59
markov00 added a commit to nickofthyme/kibana that referenced this pull request Dec 20, 2021
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 enhancement New value added to drive a business result Feature:Discover Discover Application Feature:ElasticCharts Issues related to the elastic-charts library Feature:Lens Feature:TSVB TSVB (Time Series Visual Builder) Feature:XYAxis XY-Axis charts (bar, area, line) release_note:enhancement v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants