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

[TSVB] Enable dual mode, support index patterns and strings #92812

Merged
merged 61 commits into from
Mar 29, 2021

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Feb 25, 2021

Closes: #91367, Closes: #93751, Closes: #80849

Release notes

Adding an additional mode for selecting the pattern index for TSVB. Now, in addition to entering the index in string form, the user can select the desired index using the drop-down list.

image

The mode is switched by pressing the "Gear" button:

image

Summary

Describe the feature:

TSVB right now doesn't work entirely with index patterns. Specifically, it allows the user to enter a string (which can be the index pattern title, an index, multiple indices, wildcards etc).

image

This has served us well so far but we have identified some cases that is not going to work well in the future. For example:

  • we will have to support runtime fields with a hacky way
  • if the index patterns work with non-unique pattern lists in the future, it will be impossible for TSVB to identify which index pattern the user wants to use (Imagine that the user has created 2 index patterns with the same pattern list but only one of them supports runtime fields).

For all the above we decided to enable a dual mode in TSVB. This means that we will keep the old implementation, meaning that we will be backwards compatible and the user will be able to create a visualization by giving a string but we will also give the user the ability to create a visualization from the index pattern.

@alexwizp
Copy link
Contributor Author

alexwizp commented Mar 1, 2021

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

@elastic/kibana-design can you help us with that? Our goal here is to have two ways to configure a TSVB visualizations. The old one with the input string and the new one with a combobox. We want to highlight more the new implementation as the old one is going to be deprecated in the future. Aliaksei has made a first attempt but it would help us a lot to also have your feedback!

@ryankeairns
Copy link
Contributor

I'll take a look from the design side.

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

In general, this feels like a good approach that needs just a couple of adjustments.

While I appreciate the desire to push the new way versus the old way, the warning icon feels a bit too distracting (some may still prefer the old way, for reasons, and would be nagged by this icon).

Since we are already defaulting to the new way, I would suggest adding the description (text from the warning callout) to the settings popover in context with the switch. To clarify, I don't think we need the actual callout container, rather just move the message from there to this popover.

A pattern to follow would be the KQL popover on the filter/search bar. Add a panel title, explanatory text, and the switch below.

Screen Shot 2021-03-01 at 2 03 34 PM

For the switch itself, in my opinion, the title above is not necessary if we use a more descriptive label like this:
Screen Shot 2021-03-01 at 2 19 45 PM

Let me know if you have questions.

@alexwizp
Copy link
Contributor Author

alexwizp commented Mar 2, 2021

@ryankeairns thank you for your review, I've updated UI, what do you think about it now
image

image

image

@ryankeairns
Copy link
Contributor

@alexwizp thank you for the changes!

The callout in the popover is not a pattern we've used elsewhere and is not one I'd like to set. Further, adding a separate button/action in this popover seems like something we can avoid.

What do you think about simply clearing out the input if you toggle the switch? That would avoid us having to handle this messaging altogether and, in my opinion, seems a reasonable expectation if a user is setting a new index type.

The current callouts strike me moreso as validation messages. If there are cases where these still arise (say we clear the text, close the popover, and then they type an unidentified index), then we can present this copy as validation/error text below the input. Thoughts?

@alexwizp
Copy link
Contributor Author

alexwizp commented Mar 2, 2021

@ryankeairns I don't have a clear opinion on this. Yes, we can remove callouts. But on the other hand, it allows you to quickly navigate to the pattern index creation page ...

@stratoula @timroes maybe you have some ideas?

@alexwizp
Copy link
Contributor Author

alexwizp commented Mar 3, 2021

@elasticmachine merge upstream

@elastic elastic deleted a comment from kibanamachine Mar 3, 2021
@alexwizp
Copy link
Contributor Author

alexwizp commented Mar 3, 2021

@ryankeairns I removed all callouts. Now it's just a popover with text and switch like we have for kql.

@elastic elastic deleted a comment from kibanamachine Mar 3, 2021
@alexwizp
Copy link
Contributor Author

alexwizp commented Mar 25, 2021

@ryankeairns @stratoula what if we remove all related to 'Switch Mode'? Keep only gear icon with exciting popover. From the user's perspective, it will be a little more difficult to navigate to the creation page of the index pattern ...

@ryankeairns
Copy link
Contributor

@ryankeairns @stratoula what if we remove all related to 'Switch Mode'? Keep only gear icon with exciting popover. From the user's perspective, it will be a little more difficult to navigate to the creation page of the index pattern ...

I would prefer that, yes.
We could simply replace the 'Switch mode' link with a 'Create index pattern' link. Perhaps that's a good, small compromise here. Thanks Alexey :)

@gchaps
Copy link
Contributor

gchaps commented Mar 25, 2021

+1 on 'Create index pattern'

@crob611
Copy link
Contributor

crob611 commented Mar 25, 2021

@alexwizp @stratoula In regards to the dashboard reporting references, it should currently work as long as the embeddable factory has an extract method that properly extracts the references. Let me know if that isn't working and I'll be happy to dig in and figure out what's going on.

@stratoula
Copy link
Contributor

stratoula commented Mar 26, 2021

@ryankeairns thank you a lot for the feedback. I agree, let's change the text to 'Create index pattern' link and remove the popover. It def makes more sense. Just to have the whole picture here, we are going to remove on 8.0 the option to create viz with the input text but the old visualizations that have been created that way will work. We want the users to prefer the combobox option.

@alexwizp thank you for the fixes. It seems that the dashboard references should work, so let's address it too.

@VladLasitsa
Copy link
Contributor

VladLasitsa commented Mar 26, 2021

I found out that when we use old way by using string as index pattern we don't get fields for time field. On master it works fine. I think we should keep behavior like in master and getting list of time fields for index pattern when we use old implementation.

Already fixed

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.

LGTM, did local tests

@alexwizp
Copy link
Contributor Author

@ryankeairns

image

@stratoula stratoula added release_note:feature Makes this part of the condensed release notes and removed release_note:enhancement labels Mar 26, 2021
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for handling all the feedback!

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

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.

Code LGTM, thanx @alexwizp for all the changes, this is a great PR!

@stratoula
Copy link
Contributor

@alexwizp can you also add a brief release notes section?

@stratoula
Copy link
Contributor

@elasticmachine run elasticsearch-ci/docs

@alexwizp alexwizp enabled auto-merge (squash) March 29, 2021 08:18
@alexwizp alexwizp merged commit 4342f26 into elastic:master Mar 29, 2021
alexwizp added a commit to alexwizp/kibana that referenced this pull request Mar 29, 2021
…c#92812)

* [TSVB] Enable `dual mode`, support index patterns and strings

* modify UI

* add migration script

* refactoring

* fix CI

* prefill the index pattern name

* modify UI

* modify UI

* update UI

* fix functional test

* some work

* remove callouts

* fix rollup test

* update UI

* fix typo

* add some unit tests

* add functional test

* fix CI

* correct labels

* fix ci group 12

* cleanup interface

* fix CI

* cleanup API

* fix some of PR comments

* move index patterns into so references

* remove wrong logic

* fix JEST

* fix some ui issues

* update sample data

* indexPatternObject -> indexPatternValue

* fix comments

* I have a dashboard with two TSVB viz. One with the default (haven't applied it to the combobox) and one with the logs. The filter contains fields only from the logs index pattern

* When I am on the string mode and try to write my index, sometimes some of the chars are not added or they are deleted while typing, something with the denounce maybe?

* fix merge conflicts

* Does this PR also supports runtime fields? I created one from the editor and I see that I can select it

* fix UI issue

* If I create a viz with the string mode and a wildcard e.g. kibana_sample*, the index patterns are not communicated correctly to the dashboard.

* fix import/export refs for dashboard

* remove MigrationPopover

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
alexwizp added a commit that referenced this pull request Mar 29, 2021
#95631)

* [TSVB] Enable `dual mode`, support index patterns and strings

* modify UI

* add migration script

* refactoring

* fix CI

* prefill the index pattern name

* modify UI

* modify UI

* update UI

* fix functional test

* some work

* remove callouts

* fix rollup test

* update UI

* fix typo

* add some unit tests

* add functional test

* fix CI

* correct labels

* fix ci group 12

* cleanup interface

* fix CI

* cleanup API

* fix some of PR comments

* move index patterns into so references

* remove wrong logic

* fix JEST

* fix some ui issues

* update sample data

* indexPatternObject -> indexPatternValue

* fix comments

* I have a dashboard with two TSVB viz. One with the default (haven't applied it to the combobox) and one with the logs. The filter contains fields only from the logs index pattern

* When I am on the string mode and try to write my index, sometimes some of the chars are not added or they are deleted while typing, something with the denounce maybe?

* fix merge conflicts

* Does this PR also supports runtime fields? I created one from the editor and I see that I can select it

* fix UI issue

* If I create a viz with the string mode and a wildcard e.g. kibana_sample*, the index patterns are not communicated correctly to the dashboard.

* fix import/export refs for dashboard

* remove MigrationPopover

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTimeseries 477 485 +8
visualizations 140 143 +3
total +11

Async chunks

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

id before after diff
indexPatternManagement 554.2KB 554.3KB +84.0B
visTypeTimeseries 1.6MB 1.6MB +11.1KB
total +11.2KB

Page load bundle

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

id before after diff
visTypeTimeseries 124.4KB 126.8KB +2.5KB
visualizations 107.4KB 109.4KB +2.0KB
total +4.5KB

History

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

cc @alexwizp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:TSVB TSVB (Time Series Visual Builder) release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.13.0 v8.0.0
Projects
None yet
9 participants