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

[IndexPatterns] Support cross cluster search #11114

Merged
merged 20 commits into from
Jun 2, 2017

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Apr 8, 2017

Closes #11014
Closes #11011
Fixes #9028
Fixes #8373
Fixes #9386
FIxes #9028
... there are probably others

Summary:

image

Index Patterns can now point to indices from remote clusters when using Elasticsearch's cross cluster search feature. After setting it up in elasticsearch, just mention the remote cluster alias in the index name like so: remoteCluster:indexName.


When using cross cluster search, we have to pull the field definitions for index patterns in a cross-cluster friendly way, meaning we have to move over to the new cross cluster compatible _field_caps api in elasticsearch instead of pulling _field_stats and merging them with mappings.

Technical Summary:

  • Field fetching now happens on the server:
    • GET /api/index_patterns/_fields_for_wildcard?pattern=logstash-*
      • fetches fields for the logstash-* wildcard pattern
      • fields are normalized, include kibana specific types and properties like readFromDocValues
      • fields are sorted alphabetically, ready to be stored on index pattern objects.
      • &meta_fields=["_index","_type"]
        • include a list of meta fields in the query string to add private (prefixed with an underscore) or unknown fields to the response.
    • GET /api/index_patterns/_fields_for_time_pattern?pattern=[logstash-]YYYY&look_back=10
      • will do the same thing, but for time patterns
      • Just like in master, [logstash-]YYYY is converted into a wildcard and then matched against the output of the _aliases api
      • the most recent indices (look_back determines how many) are then used with the _field_caps api
      • dependency on the _aliases endpoint means that time patterns are not cross cluster search compatible
  • Including a : in your index pattern converts it into a "cross cluster" index pattern and disables the expand wildcard and time pattern index pattern "modes"
  • Everything else is pretty much the same!

@spalger spalger force-pushed the implement/field-capabilities-api branch 5 times, most recently from 88a5df3 to 37e2460 Compare April 8, 2017 19:36
@tbragin tbragin added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Apr 10, 2017
@spalger spalger force-pushed the implement/field-capabilities-api branch 2 times, most recently from b3cf2a2 to 5c4095d Compare April 11, 2017 18:01
@spalger spalger force-pushed the implement/field-capabilities-api branch 2 times, most recently from 8f7d173 to e6f7fe2 Compare April 26, 2017 19:41
@spalger spalger changed the title [WIP] [indexPattern] use custom api endpoints to fetch fields [WIP] Implement cross cluster search May 10, 2017
@spalger spalger force-pushed the implement/field-capabilities-api branch 5 times, most recently from 4544994 to 6890ba2 Compare May 15, 2017 21:46
@s1monw
Copy link

s1monw commented May 17, 2017

@spalger I just merged #24722 FYI

@spalger spalger force-pushed the implement/field-capabilities-api branch 3 times, most recently from db157a3 to 71d6159 Compare May 18, 2017 19:50
@spalger spalger requested a review from epixa May 18, 2017 23:48
@spalger spalger changed the title [WIP] Implement cross cluster search Implement cross cluster search May 22, 2017
@spalger spalger changed the title Implement cross cluster search [IndexPatterns] Support cross cluster search May 22, 2017
@spalger spalger requested a review from Bargs May 22, 2017 18:37
@s1monw
Copy link

s1monw commented May 22, 2017

this PR changes almost 5k SLoC I really wonder if we can break it up and make smaller PRs our of this. It's impossible to review something like this in a sane way? I mean I don't know much or rather nothing about javascript but this is pretty massive?

@Bargs
Copy link
Contributor

Bargs commented May 22, 2017

Yeah it's quite large... I'm wondering, does the move from _field_stats to _field_caps necessitate a move from the front end to the back end? I'm just thinking, if we're going to remove date patterns and expanded index patterns in the somewhat near future, it might be easier to move mapping handling to the server side after that since there will be less legacy code to replicate? I'm not terribly familiar with the requirements for cross cluster search though, so there may be reasons I'm missing.

Including a : in your index pattern converts it into a "cross cluster" index pattern and disables the expand wildcard and time pattern index pattern "modes"

What does a "cross cluster" index pattern imply? Is it just that those modes get disabled, or is there more to it? Why a : instead of a boolean flag or something?

@spalger
Copy link
Contributor Author

spalger commented May 22, 2017

does the move from _field_stats to _field_caps necessitate a move from the front end to the back end?

It doesn't anymore. We thought it would until elastic/elasticsearch#24463, when it was a serious possibility that Kibana was going to have to connect to all of the clusters and get field_caps from them independently.

I'm just thinking, if we're going to remove date patterns and expanded index patterns in the somewhat near future, it might be easier to move mapping handling to the server side after that since there will be less legacy code to replicate?

That's probably possible, but @epixa and I decided to do it now because we needed to be ready for the situation in my first answer. The server-side part of this PR is about 500 lines of code, 15% of the PR, so I really don't think it saves us that much to reimplement this as client-side only and move it to the server later.

What does a "cross cluster" index pattern imply? Is it just that those modes get disabled, or is there more to it? Why a : instead of a boolean flag or something?

When an index pattern is cross cluster Kibana can't talk directly to the clusters that have the data, it can only use the _search and _field_caps APIs. The : is just elasticsearch notation, and seemed like a natural way to specify which clusters to use.

@@ -186,11 +186,8 @@ export function SegmentedRequestProvider(es, Private, Promise, timefilter, confi

return indexPattern.toDetailedIndexList(timeBounds.min, timeBounds.max, this._direction)
.then(queue => {
if (!_.isArray(queue)) queue = [queue];
Copy link
Contributor

Choose a reason for hiding this comment

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

When was this condition hit before this PR?

Copy link
Contributor Author

@spalger spalger May 31, 2017

Choose a reason for hiding this comment

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

toDetailedIndexList() previously returned an array when there were multiple indexes, and a single object if not, but now it just always returns an array with 1 or more objects in it.

import { resolve as resolveUrl, format as formatUrl } from 'url';

import { pick, mapValues } from 'lodash';
import 'whatwg-fetch';
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't, removing. I was originally planning to use it, but using the $http service helps this client integrate better with existing code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use this module anywhere else? Can we just remove it from package.json entirely?

@spalger
Copy link
Contributor Author

spalger commented May 31, 2017

Can you describe the purpose around the timePatternToWildcard logic a bit? I'm not sure I understand why it needs to happen here.

The timePatternToWildcard() helper converts the [logstash-]YYYY.MM.DD time patterns into logstash-* so we can get the aliases for all indices matching that pattern, and then use that list to determine which indices match the date pattern by attempting to parse the returned index aliases using the time pattern as the format: moment(indexName, timePattern, true /* strict mode */)

This allows us to see which of the existing indices will match when we use the time pattern to create index names based on the time filter, and most importantly allows us to pull field capabilities for indices we know exist, rather than just generating 10 or so index names and hoping one or more of them exists.

@epixa
Copy link
Contributor

epixa commented Jun 2, 2017

When I specify a CCS enabled index pattern during creation, the Expand index pattern when searching option is now visible and selectable. That's not intentional, I assume?

Copy link
Contributor

@kimjoar kimjoar left a comment

Choose a reason for hiding this comment

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

Changes LGTM, and except for what Court mentions, everything works as expected for me.

Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

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

LGTM pending the fix of that last comment of mine

@spalger spalger merged commit e5deca6 into elastic:master Jun 2, 2017
@spalger
Copy link
Contributor Author

spalger commented Jun 2, 2017

💃 💃 💃 💃 💃 💃 💃 💃 💃

spalger added a commit to spalger/kibana that referenced this pull request Jun 2, 2017
* [indexPatterns] support cross cluster patterns

* [vis] remove unused `hasTimeField` param

* [indexPatterns/create] fix method name in view

* [indexPatterns/create] disallow expanding with ccs

* [indexPatterns/create] field fetching is cheaper, react faster

* [indexPatterns/resolveTimePattern/tests] increase readability

* [tests/apiIntegration/indexPatterns] test conflict field output

* [indexPatterns/fieldCaps/readFieldCapsResponse] add unit tests

* [test/apiIntegration] ensure random word will not be valid

* [indexPatterns/ui/client] remove unused import

* remove use of auto-release-sinon

* [indexPatterns/create] don't allow expand when cross cluster

* [indexPatternsApiClient/stub] use angular promises

* [indexPatterns/create] add tests for base create ui behaviors

(cherry picked from commit e5deca6)
@epixa
Copy link
Contributor

epixa commented Jun 2, 2017

🎉

spalger added a commit that referenced this pull request Jun 2, 2017
* [indexPatterns] support cross cluster patterns

* [vis] remove unused `hasTimeField` param

* [indexPatterns/create] fix method name in view

* [indexPatterns/create] disallow expanding with ccs

* [indexPatterns/create] field fetching is cheaper, react faster

* [indexPatterns/resolveTimePattern/tests] increase readability

* [tests/apiIntegration/indexPatterns] test conflict field output

* [indexPatterns/fieldCaps/readFieldCapsResponse] add unit tests

* [test/apiIntegration] ensure random word will not be valid

* [indexPatterns/ui/client] remove unused import

* remove use of auto-release-sinon

* [indexPatterns/create] don't allow expand when cross cluster

* [indexPatternsApiClient/stub] use angular promises

* [indexPatterns/create] add tests for base create ui behaviors

(cherry picked from commit e5deca6)
@epixa epixa mentioned this pull request Jun 13, 2017
3 tasks
@spalger spalger deleted the implement/field-capabilities-api branch October 18, 2019 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v5.5.0 v6.0.0-beta1 v6.0.0
Projects
None yet
7 participants