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

fix: change es version check to support elasticsearch 8+ #4512

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kjschnei001
Copy link

@kjschnei001 kjschnei001 commented Jun 6, 2023

Which problem is this PR solving?

  • Allows for writing of spans to a v8 elasticsearch index.
  • I am utilizing the library only for writing spans to an elasticsearch instance, so I can't confirm that these changes are sufficient for elasticsearch 8+ support in jaeger, but I do know that the _type field is incompatible in elasticsearch v8, and removing it allows me to successfully write to an index in elasticsearch v8.
  • Even if it doesn't fully satisfy the requirements for v8 support, it does feel like a very safe change to make, as it doesn't modify the logic for v7 and lower versions of elasticsearch.

Short description of the changes

  • This PR modifies the version check for elasticsearch to apply v7 logic to any version equal to or greater than v7.

@kjschnei001 kjschnei001 requested a review from a team as a code owner June 6, 2023 20:26
Signed-off-by: KevinSchneider <kevin.schneider@target.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@codecov
Copy link

codecov bot commented Jun 11, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01 🎉

Comparison is base (f282d9b) 97.03% compared to head (ff35d00) 97.04%.

❗ Current head ff35d00 differs from pull request most recent head 93f3151. Consider uploading reports for the commit 93f3151 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4512      +/-   ##
==========================================
+ Coverage   97.03%   97.04%   +0.01%     
==========================================
  Files         301      300       -1     
  Lines       17817    17813       -4     
==========================================
- Hits        17289    17287       -2     
+ Misses        423      421       -2     
  Partials      105      105              
Flag Coverage Δ
unittests 97.04% <ø> (+0.01%) ⬆️

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

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yurishkuro
Copy link
Member

The change needs to be demonstrated with CI running successfully against v8. I added it to the matrix, but I think there is some infra issue where a started DB is not recognized by our script, needs investigation.

Signed-off-by: KevinSchneider <kevin.schneider@target.com>
Signed-off-by: KevinSchneider <kevin.schneider@target.com>
Signed-off-by: KevinSchneider <kevin.schneider@target.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants