[Management] Index pattern creation wizard enhancements#13960
[Management] Index pattern creation wizard enhancements#13960chrisronline merged 6 commits intoelastic:masterfrom
Conversation
- Ensure consistent counts by searching for twice as many as we show in the UI, this should avoid count issues due to system indices filtering (elastic#13735)
| return indices.filter(index => !index.name.startsWith('.')); | ||
| const filtered = indices.filter(index => !index.name.startsWith('.')); | ||
| if (filtered.length > MAX_NUMBER_OF_MATCHING_INDICES) { | ||
| return filtered.slice(0, MAX_NUMBER_OF_MATCHING_INDICES); |
There was a problem hiding this comment.
Thoughts on removing the if block and always return the slice?
const filtered = indices.filter(index => !index.name.startsWith('.'));
return filtered.slice(0, MAX_NUMBER_OF_MATCHING_INDICES);
There was a problem hiding this comment.
I can do that!
|
Also, since we are now putting a limit on the return - should we have an indication in the UI that there might be more results? |
|
@tylersmalley I'm not sure I follow. We've always had a limit on the return. Are you suggesting that because we have a limit, we should indicate to the user that more might exist? Definitely good feedback @alexfrancoeur |
|
I see, we're setting upper bounds since we are filtering internal indices. Yes, I believe that if we ever limit results that we need some indication to the user that they are limited. It's outside the scope of this PR and I will let @alexfrancoeur weigh in on it. |
|
@tylersmalley All fixed up! |
|
Build failure is due to S3 upload, so otherwise successful - #13786 |
|
@tylersmalley @chrisronline I agree with acknowledging that there are more indices if we are limiting the results. I honestly didn't know we were. Are we excluding more than just the internal indices? If so we could take a similar approach that discover does showing the total number of hits and then only presenting 500 documents. You can imagine a descriptive sentence similar to this:
^^ That's a bit overkill but was the first thing that came to mind. It seems that we should call out the indices shown are only for the local cluster unless a CCS index pattern is defined. I'm also skeptical about adding more text to this page, the goal of this new UI was to slim it down. |
|
Roping in @cjcenizal for some thoughts here. I'm working on a proto version of what Alex is describing, but curious to other thoughts before I go too far with it. |
|
@chrisronline @alexfrancoeur, since we are already limiting the results, how about moving forward with getting this in and opening up a new PR to communicate the truncation? |
|
I can get down with that! |
|
It's been awhile since my head has been in this code, but I think the idea behind filtering the results was that beyond a certain number of results, this information just becomes noise -- what does it matter if you match 20, 100, or 1,000 indices? There's probably going to be a pattern to how you've named your indices so once you match ~20, you have a good idea of the others you'll match. Or so the thinking went. So the idea is that this is just a sample of the matching indices, so you know that you're matching something and you have a general idea of which kinds of indices you're matching. If that's the idea, then how do we best communicate it? I think the best way would be to show a very short message (hat tip to Alex's point about keeping text to a minimum at this point), e.g. "Representative sample", which hopefully gets the point across. I'd use Thoughts? |
|
Next to the pagination controls? |
|
@chrisronline Yep, e.g. "1-10 of 40 (representative sample only)" |
|
@cjcenizal Added! |
|
This good to go in? |
|
Just looking for a second reviewer, then yes |
|
I'm reviewing this right now. 🔍 |
cjcenizal
left a comment
There was a problem hiding this comment.
Looking really good! I just have a few suggestions.
|
|
||
| // All system indices begin with a period. | ||
| return indices.filter(index => !index.name.startsWith('.')); | ||
| return indices.filter(index => !index.name.startsWith('.')).slice(0, MAX_NUMBER_OF_MATCHING_INDICES); |
There was a problem hiding this comment.
This will only apply the cap to queries which exclude system indices. Wouldn't we want to apply it to queries which also include system indices? Something like this:
const whiteListIndices = indices => {
if (!indices) {
return indices;
}
const acceptableIndices =
this.doesIncludeSystemIndices
? indices
// Exclude system indices, which begin with a period.
: indices.filter(index => !index.name.startsWith('.'));
return acceptableIndices.slice(0, MAX_NUMBER_OF_MATCHING_INDICES);
};| Notifier, | ||
| Promise | ||
| ) { | ||
| const MAX_SEARCH_SIZE = 40; |
There was a problem hiding this comment.
Why did we choose 40 again? Might be worth a brief comment here.
| this.fetchExistingIndices = () => { | ||
| this.isFetchingExistingIndices = true; | ||
| const allExistingLocalAndRemoteIndicesPattern = '*,*:*'; | ||
| const allExistingLocalAndRemoteIndicesPattern = '*'; |
There was a problem hiding this comment.
Does this variable name still make sense? If I recall correctly, the *,*:* pattern is a cross-cluster search pattern, which is what the "remote" indices part of the variable name refers to. So maybe this variable name now needs to be just allExistingIndices?
| ng-model-options="{ updateOn: 'default blur', debounce: {'default': 500, 'blur': 0} }" | ||
| validate-index-pattern | ||
| allow-wildcard | ||
| validate-index-pattern-allow-wildcard |
|
|
||
| <span | ||
| ng-if="matchingIndicesList.hasMultiplePages()" | ||
| className="kuiSubduedText" |
There was a problem hiding this comment.
You've been living in React-land for too long my friend. This should be class, not className. 😄
There was a problem hiding this comment.
hahaha oh man nice find
|
@cjcenizal All set for another pass! |
|
Looks like there's a failing dashboard test. I pinged @stacey-gammon about it. |
|
Failure: with screenshot: It seems like when clicking to Visualize and back to dashboard, the landing page is loading instead of the previously opened dashboard. Doesn't look like your PR changes should have caused this. lets run again, and if it's flaky, I'll file an issue for it. Jenkins, test this |
* Ensure we allow wildcards, #13956 * - Only search a single cluster by default (#13689) - Ensure consistent counts by searching for twice as many as we show in the UI, this should avoid count issues due to system indices filtering (#13735) * PR feedback * Just return it * Add representative sample copy * PR feedback
|
Backport: 6.x: 2ce5608 |
Fixes #13956
Fixes #13689
Partially fixes #13735
#13956 is a straightforward fix.
For #13689, I changed the default search query from
*,*:*to just*. This will only perform a single cluster search. Users should be able to manually type in a query that will search a single cluster, or across all of them (<clusterName>:<index_name>). @spalger Does that sound right?For #13735, I think the first issue is because we are limiting our ES query to 20 results, but filtering those results based on system indices. It becomes possible that one search will show
14 resultswhereas the next one (which is more specific) could show19 results. I've changed it so we double how many ES returns and filter it down to 20. I've tested this through a few scenarios (like the above mentioned one) and the counts look right to me.@alexfrancoeur