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] Add control for global filters to the annotation layer menu #141615

Merged
merged 21 commits into from
Sep 30, 2022

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Sep 23, 2022

Summary

Closes #140204

This PR refactors the layer actions menu code and provide a way to each visualization to list custom actions when needed.
The first custom action to ship is the Ignore global filters in the annotation layer.

Screenshot 2022-09-23 at 12 28 28

Screenshot 2022-09-23 at 12 56 00

I've used the same visibility icon for the action, and modelled the UI as two distinct actions (Ignore vs Keep).

ignore_filters_button

TSVB to Lens

The TSVB to Lens has been updated to create layers based on dataView used AND the ignoreGlobalFilters flag.

Bug fixes

A fix has been introduced for the scenario where the user didn't specify a dataView, as in TSVB this has a fallback to the default one. The fallback wasn't picked up by the conversion code originally, but now it is (added test for this).

Discussion points

  • There's no actual way from the user perspective to know whether the filters are ignored or kept than open the action menu and check what's the current state/label in there. Is that ok? cc @ghudgins
  • The icon user for the actual reflects the same icon(s) used to toggle the state of filters in Kibana. Is there anything better? cc @MichaelMarcialis
  • The current feature state is modelled as two distinct actions. Is that ok?
    • A possible alternative would be to have a toggle switch on the same dropdown, keeping the same label. cc @MichaelMarcialis

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@dej611 dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens v8.6.0 labels Sep 23, 2022
@dej611 dej611 changed the title [Lens] Add ignore global filters action to the annotation layer menu [Lens] Add control for global filters to the annotation layer menu Sep 23, 2022
@dej611 dej611 marked this pull request as ready for review September 28, 2022 12:30
@dej611 dej611 requested a review from a team as a code owner September 28, 2022 12:30
@elasticmachine
Copy link
Contributor

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

model.annotations,
(a) => typeof a.index_pattern === 'object' && 'id' in a.index_pattern && a.index_pattern.id
);
const annotationsByIndexPatternAndIgnoreFlag = groupBy(model.annotations, (a) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

😍👌🏼

@MichaelMarcialis
Copy link
Contributor

  • The icon user for the actual reflects the same icon(s) used to toggle the state of filters in Kibana. Is there anything better? cc @MichaelMarcialis

Yes! I'm planning to create a filter-specific ignore icon tomorrow and get it added to EUI (along with a handful of other Lens icons). Stay tuned. I'll CC you on the EUI PR.

  • A possible alternative would be to have a toggle switch on the same dropdown, keeping the same label. cc @MichaelMarcialis

For now, my suggestion is that we keep it as you currently have it (with two distinct context menu actions) for the sake of aesthetics and simplicity, rather than using an EuiSwitch. I'm anticipating the number of layer-level actions growing quite quickly over the next few months; enough so that I imagine it will eventually outgrow a popover (and likely require a dedicated layer configuration flyout or modal). Until we hit that critical threshold though, I'd like to try to stick to a regular old context menu for as long as possible.

@dej611
Copy link
Contributor Author

dej611 commented Sep 30, 2022

@MichaelMarcialis if that's ok for you I would like to merge this PR and then update the icon as follow up task when ready. would that be ok?

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 checked it before and it works fine. However, I think the big problem is what you mentioned in the description (user not knowing if the annotation group keeps or ignores global filters). If we decide to solve it later, let me know.

Please solve the conflicts and I'll recheck and approve :)

Testing log:

  • Lens to TSVB:
    • an annotation with ignoreGlobalFilters: true
    • an annotation with ignoreGlobalFilters: false
    • multi annotations with different settings that influence how the essags are grouped and ran:
      • indexPattern, timeField, ignoreGlobalFilters
  • Lens annotations creation:
    • an annotation group with ignoreGlobalFilters: true
    • an annotation group with ignoreGlobalFilters: false
    • two annotations groups with the same dataView and same timeField but different ignoreGlobalFilters setting
    • moving an annotation from the group with ignoreGlobalFilters: true to ignoreGlobalFilters: false

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 917 918 +1

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
lens 565 567 +2

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 +1.3KB
visTypeTimeseries 493.2KB 493.5KB +312.0B
total +1.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
lens 43 45 +2
Unknown metric groups

API count

id before after diff
lens 654 658 +4

History

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

@MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis if that's ok for you I would like to merge this PR and then update the icon as follow up task when ready. would that be ok?

Sounds good. I'll CC you when the icon is ready.

@dej611 dej611 merged commit 93b97bf into elastic:main Sep 30, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 30, 2022
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 2, 2022
…lastic#141615)

* ✨ Add ignore global filters feature

* ✅ Fix tests

* 🔧 Remove unused translations

* ♻️ Make it simpler

* ✅ Fix test

* 🔧 slighlty increase bundle limit size

* 🔧 Forward layer flag to event config

* ♻️ refactor actions code to include viz custom actions

* 🐛 Flip the logic

* 🔥 Remove unused file

* ✨ Migrate ignore flag to annotation layers

* ✅ Add unit test for default dataView
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":"#146471
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":"#146471
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 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.

[Lens][Query annotations] Add "ignore global filters" option
6 participants