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][Agg based Gauge & Goal] Convert to Lens Agg based Gauge and Goal. #142838

Merged
merged 50 commits into from
Oct 11, 2022

Conversation

Kuznietsov
Copy link
Contributor

@Kuznietsov Kuznietsov commented Oct 6, 2022

Summary

Completes part of #138236

As part of phasing out TSVB and Visualize all Legacy agg based visulizations should support "open in lens" functionality.
In that PR converter for Agg based Gauge and Goal were added.

@Kuznietsov Kuznietsov added the WIP Work in progress label Oct 6, 2022
@Kuznietsov Kuznietsov self-assigned this Oct 6, 2022
# Conflicts:
#	x-pack/plugins/lens/public/visualizations/gauge/visualization.tsx
@Kuznietsov Kuznietsov removed the WIP Work in progress label Oct 10, 2022
@Kuznietsov Kuznietsov changed the title [WIP][Lens][Agg based Gauge & Goal] Convert to Lens Agg based Gauge and Goal. [Lens][Agg based Gauge & Goal] Convert to Lens Agg based Gauge and Goal. Oct 10, 2022
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Thanx Yaroslav for this PR! I found the following bug:

  • All the sibling pipeline aggregations navigate to Lens with an error (Cannot read properties of undefined)

image

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

There's a small difference about the color ranges between Lens and Visualize Goal. In Visualize, when you have a value higher than the upper range, it takes the last color. In Lens, if you have a value higher than the upper range, it doesn't get colored. Should we address it?

Screenshot 2022-10-11 at 10 00 45

Screenshot 2022-10-11 at 10 00 29

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

I have no opinion about the issue I mentioned before (the difference about overflown values), maybe it actually works better for Lens. Whatever you decide (to correct it or not) I am good with it. Approved 👌🏼

@Kuznietsov
Copy link
Contributor Author

@mbondyra, I think, we should not, because it would be an undefined behavior 😄

@stratoula, WDYT?

@stratoula
Copy link
Contributor

@Kunzetsov @mbondyra yes I think it is ok.

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

I tested it again and everything works fine. LGTM in case of green CI!

@Kuznietsov
Copy link
Contributor Author

@mbondyra, @stratoula, thanks for the review and approval)

@Kuznietsov Kuznietsov enabled auto-merge (squash) October 11, 2022 08:42
@flash1293
Copy link
Contributor

It seems like the Goals panel from the logs sample dashboard is not convertable - am I missing something?
Screenshot 2022-10-11 at 09 52 42

@Kuznietsov Kuznietsov disabled auto-merge October 11, 2022 08:59
Copy link
Contributor

@VladLasitsa VladLasitsa left a comment

Choose a reason for hiding this comment

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

code review only, LGTM!

@flash1293
Copy link
Contributor

I'm debating a bit whether gauges with ranges should become Lens gauges or the new metric vis:

Let's take this gauge
Screenshot 2022-10-11 at 11 59 21

This is how it converts today
Screenshot 2022-10-11 at 11 59 25

We could also do this:
Screenshot 2022-10-11 at 12 00 48

Two problems I can imagine:

  • New metric vis doesn't allow a min value that's not 0
  • In the new metric vis the ranges are not visualized (only the status of the current value)

Nice part:

  • Subjectively looks better
  • Very prominently show the current number like the legacy vis

What do you think @gvnmagni @markov00 ? What's better at capturing the intent of the user?

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeGauge 19 28 +9
visTypeMetric 27 16 -11
visualizations 360 373 +13
total +11

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
expressionGauge 57 58 +1
visualizations 699 724 +25
total +26

Async chunks

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

id before after diff
lens 1.3MB 1.3MB +270.0B
visTypeGauge 6.1KB 7.1KB +1.0KB
visTypeMetric 23.5KB 464.0B -23.1KB
visualizations 240.2KB 263.9KB +23.7KB
total +1.9KB

Page load bundle

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

id before after diff
expressionGauge 14.0KB 14.2KB +193.0B
visTypeGauge 9.7KB 13.8KB +4.2KB
visTypeMetric 13.0KB 12.7KB -285.0B
visualizations 52.1KB 53.4KB +1.3KB
total +5.4KB
Unknown metric groups

API count

id before after diff
expressionGauge 57 58 +1
visualizations 729 754 +25
total +26

async chunk count

id before after diff
visTypeGauge 1 3 +2

History

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

cc @Kunzetsov

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.

Discussed with the datavis team and I think we can go with the current conversion logic for now. LGTM

@Kuznietsov Kuznietsov merged commit 87e5713 into elastic:main Oct 11, 2022
kibanamachine added a commit that referenced this pull request Dec 8, 2022
# Backport

This will backport the following commits from `main` to `8.6`:
- [Adds the VisEditor docs for 8.6
(#146471)](#146471)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Kaarina
Tungseth","email":"kaarina.tungseth@elastic.co"},"sourceCommit":{"committedDate":"2022-12-08T20:18:03Z","message":"Adds
the VisEditor docs for 8.6 (#146471)\n\n## Summary\r\n\r\nAdds the 8.6
for the following:\r\n\r\n- #140878, #143946 and
#142187\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/tsvb.html#edit-visualizations-in-lens)\r\n\r\n-
#142936, #142561, #143820, and
#142838\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/add-aggregation-based-visualization-panels.html#edit-agg-based-visualizations-in-lens)\r\n\r\n-
#138732\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#change-the-fields)\r\n\r\n-
#141626 and #141615\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations)\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Stratoula Kalafateli
<stratoula1@gmail.com>","sha":"f918a3745be3badff9cf05950db61d7f47877961","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Docs","release_note:skip","v8.6.0","v8.7.0"],"number":146471,"url":"https://github.com/elastic/kibana/pull/146471","mergeCommit":{"message":"Adds
the VisEditor docs for 8.6 (#146471)\n\n## Summary\r\n\r\nAdds the 8.6
for the following:\r\n\r\n- #140878, #143946 and
#142187\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/tsvb.html#edit-visualizations-in-lens)\r\n\r\n-
#142936, #142561, #143820, and
#142838\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/add-aggregation-based-visualization-panels.html#edit-agg-based-visualizations-in-lens)\r\n\r\n-
#138732\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#change-the-fields)\r\n\r\n-
#141626 and #141615\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations)\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Stratoula Kalafateli
<stratoula1@gmail.com>","sha":"f918a3745be3badff9cf05950db61d7f47877961"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/146471","number":146471,"mergeCommit":{"message":"Adds
the VisEditor docs for 8.6 (#146471)\n\n## Summary\r\n\r\nAdds the 8.6
for the following:\r\n\r\n- #140878, #143946 and
#142187\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/tsvb.html#edit-visualizations-in-lens)\r\n\r\n-
#142936, #142561, #143820, and
#142838\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/add-aggregation-based-visualization-panels.html#edit-agg-based-visualizations-in-lens)\r\n\r\n-
#138732\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#change-the-fields)\r\n\r\n-
#141626 and #141615\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations)\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Stratoula Kalafateli
<stratoula1@gmail.com>","sha":"f918a3745be3badff9cf05950db61d7f47877961"}}]}]
BACKPORT-->

Co-authored-by: Kaarina Tungseth <kaarina.tungseth@elastic.co>
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 Feature:Vis Editor Visualization editor issues impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants