-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add Elasticsearch 7 support #1690
Add Elasticsearch 7 support #1690
Conversation
d646855
to
1f1b304
Compare
Codecov Report
@@ Coverage Diff @@
## master #1690 +/- ##
=========================================
Coverage ? 98.22%
=========================================
Files ? 195
Lines ? 9529
Branches ? 0
=========================================
Hits ? 9360
Misses ? 134
Partials ? 35
Continue to review full report at Codecov.
|
Elasticsearch 7 documentation
I have tested
Data migration from ES6 to ES7
I was able to migrate data from ES 5.x up to ES 7.x. I did the following
Mapping changes:
Other changes which affect usOne span can have only 10000 tags and logs (sum) by default
Warn logs in ES7
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregoryfranklin thanks for the nice work!
A couple of high-level comments
- avoid the use of deprecated APIs in ES7 - see inline comments
- keep only two sets of index mappings - one for ES7 and another for ES6 and ES5
Also the PR needs to be rebased
plugin/storage/es/esRollover.py
Outdated
print('Creating index template {}'.format(template_name)) | ||
headers = {'Content-Type': 'application/json'} | ||
s = get_request_session(os.getenv("ES_USERNAME"), os.getenv("ES_PASSWORD"), str2bool(os.getenv("ES_TLS", 'false')), os.getenv("ES_TLS_CA"), os.getenv("ES_TLS_CERT"), os.getenv("ES_TLS_KEY")) | ||
r = s.put(sys.argv[2] + '/_template/' + template_name, headers=headers, data=template) | ||
compat = '?include_type_name=true' if esVersion == '7' else '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my note:
Before 7.0.0, the mappings definition used to include a type name. Although mappings no longer contain a type name by default, you can still use the old format by setting the parameter include_type_name. For more details, please see Removal of mapping types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The include_type_name parameter in the index creation, index template, and mapping APIs will default to false. Setting the parameter at all will result in a deprecation warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again do we need this parameter? It is already deprecated in ES7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is recommended to make index templates typeless by re-adding them with include_type_name set to false. Under the hood, typeless templates will use the dummy type _doc when creating indices.
return WrapESSearchService(c.client.Search(indices...)) | ||
searchService := c.client.Search(indices...) | ||
if c.esVersion == 7 { | ||
searchService = searchService.RestTotalHitsAsInt(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
rest_total_hits_as_int or restTotalHitsAsInt | boolean - Indicates whether hits.total should be rendered as an integer or an object in the rest search response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, rest_total_hits_as_int is required whilst using the v6 branch of github.com/olivere/elastic because the result from elasticsearch needs to be unmarshalled into a go struct.
The go struct that comes with github.com/olivere/elastic v6 matches the response from ES5 and ES6 but only matches ES7 if rest_total_hits_as_int is used.
@gregoryfranklin the PR needs to be updated and it can go in. @jaegertracing/elasticsearch please also have a look if you are interested in ES7. Or if you could review data migration part I provided in #1690 (comment) |
Based on #1690 (comment) we could use ES 7 mapping when 6.8 is detected for more seamless update. Users would just deploy with 6.8 and wait until old indices TTLout and are removed and then deploy with ES 7.x. It can be done in a separate PR. |
…s of elasticsearch A convenient way to run the elasticsearch integration test is to just run ./scripts/travis/es-integration-test.sh This change allows you to specify different elasticsearch versions eg ES_VERSION=5.6.12 scripts/travis/es-integration-test.sh ES_VERSION=6.8.1 scripts/travis/es-integration-test.sh ES_VERSION=7.2.0 scripts/travis/es-integration-test.sh The default version to use for the tests is currently 5.6.12 Signed-off-by: Greg Franklin <[email protected]>
Signed-off-by: Greg Franklin <[email protected]>
Signed-off-by: Greg Franklin <[email protected]>
Signed-off-by: Greg Franklin <[email protected]>
Signed-off-by: Greg Franklin <[email protected]>
Signed-off-by: Greg Franklin <[email protected]>
Signed-off-by: Greg Franklin <[email protected]>
…t against ES7 Signed-off-by: Greg Franklin <[email protected]>
…lates for ES7 Signed-off-by: Greg Franklin <[email protected]>
Signed-off-by: Greg Franklin <[email protected]>
Signed-off-by: Greg Franklin <[email protected]>
Signed-off-by: Greg Franklin <[email protected]>
…m to docker commands) Signed-off-by: Greg Franklin <[email protected]>
828affa
to
64a14d1
Compare
I have removed include_type_name. This also meant removing types from all the search queries. The token propagation test is currently broken. The reason for this is that Jaeger will now not start up unless it can reach elasticsearch and determine the ES version, but the token propagation test is not using a real ES server. I need to think about this a bit more to find a solution. |
We are using a mocked ES server. Just create an endpoint which returns ES version. |
Without IncludeTypeName, ES will use the type "_doc" all documents. Type is removed from all search queries so that it queries all types whether they be "_doc" or (eg) "span". Type should not be used when indexing documents on ES7 (so that the default "_doc" type is used). It should, however, be used on ES5 and ES6 so that the type matches that described by the mapping. Signed-off-by: Greg Franklin <[email protected]>
Signed-off-by: Greg Franklin <[email protected]>
…localhost Signed-off-by: Greg Franklin <[email protected]>
Signed-off-by: Greg Franklin <[email protected]>
The query service needs to detect the elasticsearch version to start up. Elasticsearch must therefore be started before the query service and must return a version number in response to a ping request Signed-off-by: Greg Franklin <[email protected]>
64a14d1
to
c878afd
Compare
plugin/storage/es/dependencystore/schema.go:37:9: if block ends with a return statement, so drop this else and outdent its block Signed-off-by: Greg Franklin <[email protected]>
@gregoryfranklin could you please merge mappings for ES 6 and ES 5 to a single file and just add a new mapping for ES 7. The legacy mapping works well for ES5 and ES6. It seems unnecessary to create duplicate files. |
… needs to be different Signed-off-by: Greg Franklin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we are getting close to the merge.
Could you please revert the change to token propagation test? And also use a single mapping for dependencies if the old mapping works?
time.Sleep(100 * time.Millisecond) | ||
|
||
// Path relative to plugin/storage/integration/token_propagation_test.go | ||
cmd := exec.Command("../../../cmd/query/query-linux", "--es.server-urls=http://127.0.0.1:9200", "--es.tls=false", "--query.bearer-token-propagation=true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please revert this. It's better to run the query from outside the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query service must be started after the elasticsearch server because the version check is done when the client is initialised (ie on startup). If you start the query service before running the test, as it was before, the query service exits with an error.
@@ -22,3 +22,17 @@ const dependenciesMapping = `{ | |||
"` + dependencyType + `":{} | |||
} | |||
}` | |||
|
|||
const dependenciesMapping7 = `{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? The legacy mapping should work with the ES7. I think you added this after I tested this and it worked without this change.
If it works please revert this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The legacy mapping does not work with ES7 because the legacy mapping includes the type name. Type names are not permitted in ES7 unless you use the include_type_name parameter, which we removed because it is deprecated.
Signed-off-by: Greg Franklin <[email protected]>
I think this looks pretty good, I will do final testing and merge tomorrow. One thing I don't like is the change to token propagation tests. To fix that we should allow starting Jaeger if the Elasticsearch version cannot be retrieved and just use one template. |
* Make it possible to run es-integration-test against different versions of elasticsearch A convenient way to run the elasticsearch integration test is to just run ./scripts/travis/es-integration-test.sh This change allows you to specify different elasticsearch versions eg ES_VERSION=5.6.12 scripts/travis/es-integration-test.sh ES_VERSION=6.8.1 scripts/travis/es-integration-test.sh ES_VERSION=7.2.0 scripts/travis/es-integration-test.sh The default version to use for the tests is currently 5.6.12 Signed-off-by: Greg Franklin <[email protected]> * Update github.com/olivere/elastic to 6.2.21 Signed-off-by: Greg Franklin <[email protected]> * Migrate to a single '_doc' elasticsearch document type Signed-off-by: Greg Franklin <[email protected]> * Add IncludeTypeName for compatability with elasticsearch 7.x Signed-off-by: Greg Franklin <[email protected]> * Remove deprecated __default__ field from es mappings Signed-off-by: Greg Franklin <[email protected]> * Rebase on master. Fix for ES5 Signed-off-by: Greg Franklin <[email protected]> * Update esRollover.py to use per-version templates Signed-off-by: Greg Franklin <[email protected]> * Update github.com/olivere/elastic to 6.2.22 and use RestTotalHitsAsInt against ES7 Signed-off-by: Greg Franklin <[email protected]> * esRollover.py should set '?include_type_name=true' when creating templates for ES7 Signed-off-by: Greg Franklin <[email protected]> * Check loading of all versions of the ES mappings Signed-off-by: Greg Franklin <[email protected]> * Run es-integration-test.sh against ES5, ES6 and ES7 Signed-off-by: Greg Franklin <[email protected]> * Run 'make fmt' to add license text to mocks Signed-off-by: Greg Franklin <[email protected]> * Update elasticsearch versions used for integration tests (and add --rm to docker commands) Signed-off-by: Greg Franklin <[email protected]> * Do not use IncludeTypeName on ES7 Without IncludeTypeName, ES will use the type "_doc" all documents. Type is removed from all search queries so that it queries all types whether they be "_doc" or (eg) "span". Type should not be used when indexing documents on ES7 (so that the default "_doc" type is used). It should, however, be used on ES5 and ES6 so that the type matches that described by the mapping. Signed-off-by: Greg Franklin <[email protected]> * Log elasticsearch version Signed-off-by: Greg Franklin <[email protected]> * Make sure we can get the elasticsearch version on servers other than localhost Signed-off-by: Greg Franklin <[email protected]> * Fix es dependency storage test Signed-off-by: Greg Franklin <[email protected]> * Fix token propagation test for elasticsearch version detection The query service needs to detect the elasticsearch version to start up. Elasticsearch must therefore be started before the query service and must return a version number in response to a ping request Signed-off-by: Greg Franklin <[email protected]> * Fix lint failure plugin/storage/es/dependencystore/schema.go:37:9: if block ends with a return statement, so drop this else and outdent its block Signed-off-by: Greg Franklin <[email protected]> * Use the same file for elasticsearch mappings on ES5 and ES6. Only ES7 needs to be different Signed-off-by: Greg Franklin <[email protected]> * Change log message to use structured logging Signed-off-by: Greg Franklin <[email protected]>
* Make it possible to run es-integration-test against different versions of elasticsearch A convenient way to run the elasticsearch integration test is to just run ./scripts/travis/es-integration-test.sh This change allows you to specify different elasticsearch versions eg ES_VERSION=5.6.12 scripts/travis/es-integration-test.sh ES_VERSION=6.8.1 scripts/travis/es-integration-test.sh ES_VERSION=7.2.0 scripts/travis/es-integration-test.sh The default version to use for the tests is currently 5.6.12 Signed-off-by: Greg Franklin <[email protected]> * Update github.com/olivere/elastic to 6.2.21 Signed-off-by: Greg Franklin <[email protected]> * Migrate to a single '_doc' elasticsearch document type Signed-off-by: Greg Franklin <[email protected]> * Add IncludeTypeName for compatability with elasticsearch 7.x Signed-off-by: Greg Franklin <[email protected]> * Remove deprecated __default__ field from es mappings Signed-off-by: Greg Franklin <[email protected]> * Rebase on master. Fix for ES5 Signed-off-by: Greg Franklin <[email protected]> * Update esRollover.py to use per-version templates Signed-off-by: Greg Franklin <[email protected]> * Update github.com/olivere/elastic to 6.2.22 and use RestTotalHitsAsInt against ES7 Signed-off-by: Greg Franklin <[email protected]> * esRollover.py should set '?include_type_name=true' when creating templates for ES7 Signed-off-by: Greg Franklin <[email protected]> * Check loading of all versions of the ES mappings Signed-off-by: Greg Franklin <[email protected]> * Run es-integration-test.sh against ES5, ES6 and ES7 Signed-off-by: Greg Franklin <[email protected]> * Run 'make fmt' to add license text to mocks Signed-off-by: Greg Franklin <[email protected]> * Update elasticsearch versions used for integration tests (and add --rm to docker commands) Signed-off-by: Greg Franklin <[email protected]> * Do not use IncludeTypeName on ES7 Without IncludeTypeName, ES will use the type "_doc" all documents. Type is removed from all search queries so that it queries all types whether they be "_doc" or (eg) "span". Type should not be used when indexing documents on ES7 (so that the default "_doc" type is used). It should, however, be used on ES5 and ES6 so that the type matches that described by the mapping. Signed-off-by: Greg Franklin <[email protected]> * Log elasticsearch version Signed-off-by: Greg Franklin <[email protected]> * Make sure we can get the elasticsearch version on servers other than localhost Signed-off-by: Greg Franklin <[email protected]> * Fix es dependency storage test Signed-off-by: Greg Franklin <[email protected]> * Fix token propagation test for elasticsearch version detection The query service needs to detect the elasticsearch version to start up. Elasticsearch must therefore be started before the query service and must return a version number in response to a ping request Signed-off-by: Greg Franklin <[email protected]> * Fix lint failure plugin/storage/es/dependencystore/schema.go:37:9: if block ends with a return statement, so drop this else and outdent its block Signed-off-by: Greg Franklin <[email protected]> * Use the same file for elasticsearch mappings on ES5 and ES6. Only ES7 needs to be different Signed-off-by: Greg Franklin <[email protected]> * Change log message to use structured logging Signed-off-by: Greg Franklin <[email protected]> Signed-off-by: kennyaz <[email protected]>
Adds support for Elasticsearch 7.x
Resolves #1474
Depends on olivere/elastic#1146
rest_total_hits_as_int support is also required for compatibility between elasticsearch 6.x and 7.x (waiting for feature to be merged in github.com/olivere/elastic) olivere/elastic#1146