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

"Create index pattern" wizard. #12689

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jul 6, 2017

Replaces #12530.

Changes

  • Create a directive for each step in the wizard.
  • Reorganize files into conventional folder structure.
  • Rename files with more conventional and consistent naming patterns.
  • Remove all translations.
  • Display indices, partial matches, exact matches.
  • Add loading, empty, and success states.
  • Add option to include system indices.

Changes to the UX

Loading indices

create_index_pattern_wizard_loading

Searching for matching indices

create_index_pattern_wizard_searching

Submitting the new index pattern

create_index_pattern_wizard_submit

To-do later

  • Implement polling for new data (and surface a spinner to show the UI is working for you).
  • Fix form validation when fields are untouched.
  • Convert directives into React components and add unit tests.
  • Improve the cross-cluster search UX.
  • Improve list of matching indices to be a table of "matching", "partially matching", and "non-matching" lists, between which the indices shuffle as the user enters the index pattern. Based on suggestions from @skearns64 and @spalger.

Feedback

From @skearns64

  • The layout doesn't strongly connect the index pattern input and the search results
  • Where are we inside of the app? We need to underline the “Index Patterns” tab
  • We should hide the sidebar when creating a new index pattern
  • The sidebar warning is visually distracting. Let’s make it clearer in location, appearance, and language. (My note: this could be replaced with an improved onboarding message).
  • Let’s remove template index patterns for now and treat it as a later optimization.

From @alexfrancoeur

I feel like we're missing some sort of graphic on the new index pattern flow. There's a lot of text that I'm worried people still skip over. What are your thoughts on providing some sort of graphic that explains an index pattern. It could be small even. ES is filled with documents, documents make up multiple indices, an index pattern is used to match against all those indices. It could almost look like a flow diagram. This might be unnecessary but I'd like to see how we can visually improve the page as well

@@ -1,139 +0,0 @@
import angular from 'angular';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this file because I think these types of assertions are better handled by a combination of functional tests and very narrowly-defined unit tests. I'll create unit tests for the new directives I've created.

@epixa
Copy link
Contributor

epixa commented Jul 6, 2017

@cjcenizal How about removing translations (from these templates) entirely? We need to do this anyway for all of the initial translations stuff when we re-approach phase 2 of the internationalization effort.

@epixa
Copy link
Contributor

epixa commented Jul 6, 2017

As an aside, that'll help with all of those literal html tags you're seeing those screenshots :)

@cjcenizal cjcenizal force-pushed the 10438/index-pattern-creation-wizard-squashed branch 2 times, most recently from 5df3c5a to cc10b00 Compare July 6, 2017 20:22
@cjcenizal cjcenizal removed the request for review from BigFunger July 6, 2017 20:38
@cjcenizal cjcenizal removed the review label Jul 6, 2017
@cjcenizal cjcenizal force-pushed the 10438/index-pattern-creation-wizard-squashed branch from 36e4caa to febb4f2 Compare July 6, 2017 21:06
@cjcenizal cjcenizal requested a review from epixa July 6, 2017 21:51
@snide
Copy link
Contributor

snide commented Jul 7, 2017

I'm seeing an empty advanced toggle when hitting step two.

image

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Some early review feedback, since we're either going to need to remove the check for indices or find some other solution before this can move forward.

class="kuiVerticalRhythm"
>
<p class="kuiText kuiStatusText kuiStatusText--error kuiVerticalRhythmSmall">
Recent changes to Elasticsearch have made this option largerly unnecessary. It will likely be removed in a future version of Kibana.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's safe to remove "likely" from this line, we're trying to figure out the soonest we can remove it right now elastic/elasticsearch#25577

id="indexPatternNameField"
class="kuiTextInput kuiTextInput--large"
data-test-subj="createIndexPatternNameInput"
ng-model="stepIndexPattern.indexPatternName"
Copy link
Contributor

Choose a reason for hiding this comment

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

I really think this needs at least a small debounce. The loading indicator flicker is quite distracting and it results in a ton of unnecessary network communication since each keystroke results in three requests. the ng-model-options strategy in master should do the trick without many changes necessary.

this.fetchExistingIndices = () => {
this.isFetchingExistingIndices = true;
Promise.all([
indicesService.getIndices('*'),
Copy link
Contributor

@spalger spalger Jul 7, 2017

Choose a reason for hiding this comment

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

This method doesn't return indices accessible via cross-cluster search, and I'm not certain we will be able to. Since the only APIs that are cross-cluster search enabled are the _search and _field_caps APIs we might be able to use a terms aggregation on _index to find all of the index names, but that probably has pretty bad performance characteristics for a request that might be executing pretty often, and anyway it's blocked on elastic/elasticsearch#25606.

- Create a directive for each step in the wizard.
- Reorganize files into conventional folder structure.
- Rename files with more conventional and consistent naming patterns.
- Improve translation key names.
- Display indices, partial matches, exact matches.
- Add loading, empty, and success states.
- Add option to include system indices.
@cjcenizal cjcenizal force-pushed the 10438/index-pattern-creation-wizard-squashed branch from 9c48ba4 to a86fddd Compare July 11, 2017 19:11
@cjcenizal
Copy link
Contributor Author

cjcenizal commented Jul 11, 2017

I'm going to put this on hold and try to get it into the next release.

CCS

Notes for testing cross-cluster search:

  1. Run npm start elasticsearch to use ESVM to install ES in the esvm directory. Kill the server that gets automatically started up.
  2. Edit esvm/dev/branch-master/config/elasticsearch.yml with these settings:
search:
    remote:
        local:
            seeds: 127.0.0.1:9300
  1. Restart the server by running ./esvm/dev/branch-master/bin/elasticsearch.
  2. Now you should be able to use a query like "local:logstash-*" to retrieve your indices via this remote.

Problems

If I used the index pattern local:k, I'd get a 500 error back from http://localhost:5601/agk/es_admin/local%3Ak/_search?allow_no_indices=true with the response:

“error”:{“root_cause”:[{“type”:“index_not_found_exception”,“reason”:“no such index”,“resource.type”:“index_or_alias”,“resource.id”:“k”,“index_uuid”:“_na_“,”index”:“k”}],“type”:“transport_exception”,“reason”:“unable to communicate with remote cluster [local]“,”caused_by”:{“type”:“index_not_found_exception”,“reason”:“no such index”,“resource.type”:“index_or_alias”,“resource.id”:“k”,“index_uuid”:“_na_“,”index”:“k”}},“status”:500}

const searchResponse = await esAdmin.search({
index: indexPattern,
ignore: [404],
allow_no_indices: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding terminate_after: 1 here will make sure that es only reads a single document from each index, meaning that the search won't need to do more work than necessary and should make this as performant as possible.

@alexfrancoeur
Copy link

I agree with @skearns64's comment - "The layout doesn't strongly connect the index pattern input and the search results" I think we can resolve this (to an extent) with suggestions from #12310 (comment)

What are your thoughts around bolding (and possibly coloring) the indices where the index pattern matches? (in the current styling)
screen shot 2017-07-18 at 4 15 06 pm

@cjcenizal
Copy link
Contributor Author

Just wanted to put a screenshot of this UI circa 5.4... so we can appreciate how far we've brought this baby in only 2 minor releases:

image

@chrisronline chrisronline self-assigned this Jul 26, 2017
@cjcenizal cjcenizal mentioned this pull request Jul 27, 2017
5 tasks
@cjcenizal
Copy link
Contributor Author

Closing in favor of #13154

@cjcenizal cjcenizal closed this Jul 27, 2017
@cjcenizal cjcenizal deleted the 10438/index-pattern-creation-wizard-squashed branch July 27, 2017 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants