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

Support Elasticsearch 8.x #4829

Merged
merged 18 commits into from Nov 23, 2023
Merged

Conversation

pmuls99
Copy link
Contributor

@pmuls99 pmuls99 commented Oct 10, 2023

Which problem is this PR solving?

Description of the changes

  • Added index templates for esv8
  • Added a esv8 client to handle the put index template request that is not currently possible by olivere/elastic
  • After these changes are merged , users no more need to use create.index-templates= false unless they want to add their custom index templates and force the es.version = 7

Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>
@pmuls99 pmuls99 requested a review from a team as a code owner October 10, 2023 03:34
@pmuls99 pmuls99 marked this pull request as draft October 10, 2023 03:58
@yurishkuro yurishkuro changed the title Support for ESV8 WIP Support for ESV8 Oct 12, 2023
pkg/es/config/config.go Outdated Show resolved Hide resolved
@yurishkuro
Copy link
Member

in the unit tests the mocks are unhappy about unexpected GetVersion call

in the CIT, ES8 complains about --env "xpack.monitoring.enabled=false" param passed from here: scripts/es-integration-test.sh

Unknown setting [xpack.monitoring.enabled] did you mean any of [xpack.profiling.enabled, xpack.monitoring.templates.enabled]?

You will need to change this flag for v8

@yurishkuro
Copy link
Member

this PR https://github.com/jaegertracing/jaeger/pull/4512/files had an example how to deal with the deprecated CLI flag. But I think we should not just remove the flag, we should change it for v8 to whatever the replacement is.

@pmuls99
Copy link
Contributor Author

pmuls99 commented Oct 27, 2023

this PR https://github.com/jaegertracing/jaeger/pull/4512/files had an example how to deal with the deprecated CLI flag. But I think we should not just remove the flag, we should change it for v8 to whatever the replacement is.

So basically this whole set of flags have been deprecated since the version 7.16(https://www.elastic.co/guide/en/elasticsearch/reference/current/monitoring-settings.html).

@pmuls99 pmuls99 changed the title WIP Support for ESV8 [WIP] Support for ESV8 Oct 27, 2023
@yurishkuro
Copy link
Member

if (( major_version < 8 )); then
    params+=(--env "xpack.monitoring.enabled=false")
else
    params+=(--env "xpack.monitoring.collection.enabled=false")
fi

@yurishkuro yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Oct 27, 2023
@yurishkuro yurishkuro self-assigned this Oct 27, 2023
plugin/storage/es/spanstore/writer.go Outdated Show resolved Hide resolved
plugin/storage/es/spanstore/writer.go Outdated Show resolved Hide resolved
plugin/storage/integration/elasticsearch_test.go Outdated Show resolved Hide resolved
scripts/es-integration-test.sh Outdated Show resolved Hide resolved
plugin/storage/es/mappings/mapping_test.go Outdated Show resolved Hide resolved
TemplateBuilder: es.TextTemplateBuilder{},
PrioritySpanTemplate: int64(c.Config.PrioritySpanTemplate),
PriorityServiceTemplate: int64(c.Config.PriorityServiceTemplate),
PriorityDependenciesTemplate: int64(c.Config.PriorityDependenciesTemplate),
Copy link
Member

Choose a reason for hiding this comment

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

please make sure PR description reflects all the changes, and explain why they are needed (especially the priority)

@yurishkuro
Copy link
Member

run make fmt

Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (9cd9fdb) 96.44% compared to head (8f9f5c6) 96.44%.

Files Patch % Lines
pkg/es/client/index_client.go 75.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4829      +/-   ##
==========================================
- Coverage   96.44%   96.44%   -0.01%     
==========================================
  Files         304      304              
  Lines       18049    18092      +43     
==========================================
+ Hits        17407    17448      +41     
- Misses        508      511       +3     
+ Partials      134      133       -1     
Flag Coverage Δ
unittests 96.44% <95.83%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yurishkuro
Copy link
Member

Please squash commits and sign

Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>
pmuls99 and others added 3 commits November 22, 2023 12:07
Signed-off-by: Yashwanth Reddy  <89122324+pmuls99@users.noreply.github.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro changed the title [WIP] Support for ESV8 Support Elasticsearch 8.x Nov 22, 2023
@yurishkuro yurishkuro marked this pull request as ready for review November 22, 2023 23:09
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro merged commit 05332d7 into jaegertracing:main Nov 23, 2023
36 checks passed
@yurishkuro
Copy link
Member

great job @pmuls99 ! 🎉

@adnankobir
Copy link

@yurishkuro when can we expect the next release?

@yurishkuro
Copy link
Member

within a week - cc @albertteoh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:new-feature Change that should be called out as new feature in CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elasticsearch 8.x support
5 participants