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 Heatmap] Navigate to Lens Agg based Heatmap. #143820

Merged
merged 33 commits into from
Oct 27, 2022

Conversation

Kuznietsov
Copy link
Contributor

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 Heatmap was added.

@Kuznietsov Kuznietsov added the WIP Work in progress label Oct 21, 2022
@Kuznietsov Kuznietsov requested a review from alexwizp October 21, 2022 15:47
@Kuznietsov Kuznietsov self-assigned this Oct 21, 2022
@Kuznietsov Kuznietsov added v8.6.0 and removed WIP Work in progress labels Oct 26, 2022
@Kuznietsov Kuznietsov marked this pull request as ready for review October 26, 2022 11:06
@Kuznietsov Kuznietsov requested a review from a team as a code owner October 26, 2022 11:06
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

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.

No empty X-axis support?

While an heatmap with no Horizontal axis defined is still an issue ( #142983 ) I think a workaround may be adopted in this specific case.

In particular I think the initial example in Heatmap agg based visualization should be exportable to Lens.
Perhaps it's a special case, but making the feature enabled only after few user changes makes it less prominent, I think.

Screenshot 2022-10-26 at 16 40 18

In this case an empty Filter dimension on the horizontal axis should be sufficient to allow for Lens exporting:

Screenshot 2022-10-26 at 16 42 07

I think the same might apply also for configurations with Y dimensions only:

Screenshot 2022-10-26 at 16 49 49

Viz based on saved searches

If the agg based on a saved search it should not show the Edit in Lens button.

Screenshot 2022-10-26 at 16 43 21

Standard deviation not supported?

Screenshot 2022-10-26 at 17 25 12

@Kuznietsov
Copy link
Contributor Author

Kuznietsov commented Oct 26, 2022

@dej611, thanks for your review. I have some questions, related to it.

No empty X-axis support?

Ok. Just want to be sure that @stratoula is approving such an approach.

Viz based on saved searches

Nobody was saying about such a special case before. Thanks for highlighting it. @stratoula, WDYT, should we make it a separate feature and move it to a separate PR?

Standard deviation not supported?

Yes, for Lens it is converting into two formulas, which is not supported by Heatmap. we can't use two value accessors. Or am I missing something?

cc @alexwizp.

@stratoula
Copy link
Contributor

@Kunzetsov thanx for pinging me

No empty X-axis support?

Yes let's go like this, sounds good to me

Viz based on saved searches

Yes let's move it on another PR. I assume that this doesn't work for any of the already merged transitions. Let's see if we can allow the transition by unlinking the saved search and go to lens with the applied filters/query. Otherwise let's block the transition for now

Standard deviation not supported

Can't we use the quick function?
image

@flash1293
Copy link
Contributor

flash1293 commented Oct 27, 2022

The agg based standard deviation is weird, it’s not actually the standard deviation, it’s two metrics with the mean minus 3 standard deviations and the mean minus 3 standard deviations.

what is agg based even rendering in that case? Two metrics on a heatmap are pretty strange

anyway, ok with not converting

@stratoula
Copy link
Contributor

The agg based standard deviation is weird, it’s not actually the standard deviation, it’s two metrics with the mean minus 3 standard deviations and the mean minus 3 standard deviations.

I din't know that 🤯 Yes let's not convert!

- Added empty filters if y-axis is defined, but x-axis is not and if no x/y-axis was defined.
- Added/fixed tests.
@Kuznietsov
Copy link
Contributor Author

@dej611, could you, please, review this PR again? I've added filters if the x-axis is not defined. Thanks for the review.

@Kuznietsov Kuznietsov requested a review from dej611 October 27, 2022 10:41
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.

Found a couple more issues when testing latest version.

Histogram issues

There are two issues with this histogram configuration:

  • the Show empty buckets flag is disabled in Visualized but enabled in Lens
  • when in Lens the create custom ranges is broken. Probably a misconfiguration during the transition. Removing and re-creating the custom ranges make it work again

minimum_interval_histogram

Terms Include/Exclude regexp flag enabled (minor)

Definetly a minor issue, but for some reason the regexp toggles of the Include/Exclude fields when in Lens are enabled, even when the fields are not used in Visualize
regex_enabled_lens

@Kuznietsov Kuznietsov requested a review from dej611 October 27, 2022 13:41
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.

Tested latest version and found no other issues 🎉

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeHeatmap 15 24 +9

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
expressionHeatmap 103 104 +1
visualizations 730 767 +37
total +38

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 +311.0B
visTypeHeatmap 7.5KB 7.5KB +1.0B
visualizations 268.2KB 269.4KB +1.2KB
total +1.5KB

Page load bundle

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

id before after diff
expressionHeatmap 14.2KB 14.4KB +217.0B
visTypeHeatmap 10.0KB 14.4KB +4.4KB
visTypeTable 19.5KB 19.5KB +20.0B
visualizations 53.7KB 53.8KB +76.0B
total +4.7KB
Unknown metric groups

API count

id before after diff
expressionHeatmap 107 108 +1
visualizations 760 797 +37
total +38

History

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

cc @Kunzetsov

@Kuznietsov Kuznietsov merged commit 9ae3880 into elastic:main Oct 27, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 27, 2022
* main: (24 commits)
  [Files] Add file upload to file picker (elastic#143969)
  [Security solution] Guided onboarding, alerts & cases (elastic#143598)
  [APM] Critical path for a single trace (elastic#143735)
  skip failing test suite (elastic#143933)
  [Fleet] Update GH Projects automation (elastic#144123)
  [APM] Performance fix for 'cardinality' telemetry task (elastic#144061)
  [Enterprise Search] Attach ML Inference Pipeline - Pipeline re-use (elastic#143979)
  [8.5][DOCS] Add support for differential logs (elastic#143242)
  Bump nwsapi from v2.2.0 to v2.2.2 (elastic#144001)
  [APM] Add waterfall to dependency operations (elastic#143257)
  [Shared UX] Add deprecation message to kibana react Markdown (elastic#143766)
  [Security Solution][Endpoint] Adds RBAC API checks for Blocklist (elastic#144047)
  Improve `needs-team` auto labeling regex (elastic#143787)
  [Reporting/CSV Export] _id field can not be formatted (elastic#143807)
  Adds SavedObjectsWarning to analytics results pages. (elastic#144109)
  Bump chromedriver to 107 (elastic#144073)
  Update cypress (elastic#143755)
  [Maps] nest security layers in layer group (elastic#144055)
  [Lens][Agg based Heatmap] Navigate to Lens Agg based Heatmap. (elastic#143820)
  Added support of saved search (elastic#144095)
  ...
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:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants