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

Set elasticsearch client version to >=7.17,<8.0.0 #230

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

stefan-k
Copy link
Contributor

@stefan-k stefan-k commented Feb 24, 2022

The latest versions of the Elasticsearch client have a compatibility mode which can also be used for newer server versions; however, the other way around does not work. Fixing the version to >=7.17,<8.0.0 is therefore a save option for the near future.
This commit also includes a fix for the case where "resource_status" is not part of resource_attributes, which occasionally caused crashes. If this key is not present, resource_status will be set to an empty string.
Also, newer versions of the Elasticsearch client require the "scheme" parameter to be set. By setting this already, it will be easier to eventually transition to client version 8.

@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2022

Codecov Report

Merging #230 (16a0411) into master (1c30c48) will decrease coverage by 0.04%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
- Coverage   99.34%   99.29%   -0.05%     
==========================================
  Files          54       54              
  Lines        2139     2142       +3     
==========================================
+ Hits         2125     2127       +2     
- Misses         14       15       +1     
Impacted Files Coverage Δ
tardis/plugins/elasticsearchmonitoring.py 96.66% <75.00%> (-3.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c30c48...16a0411. Read the comment docs.

@giffels giffels requested review from a team, giffels and RHofsaess and removed request for a team February 24, 2022 09:57
@giffels giffels added the Improvement Code Improvements label Feb 24, 2022
@giffels giffels added this to the 0.7.0 - Release milestone Feb 24, 2022
@giffels
Copy link
Member

giffels commented Feb 24, 2022

Hi @stefan-k, thanks a lot for your contribution.

Do you have any idea why resource_status is occassionlly not part of resource_attributes?

Best regards,
Manuel

Copy link
Member

@giffels giffels left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution. Apart from my inline comment, I noticed that the tests have not been updated to cover the new behaviour. Could you add a corresponding test case, please?

tardis/plugins/elasticsearchmonitoring.py Outdated Show resolved Hide resolved
@maxfischer2781
Copy link
Member

@stefan-k @giffels This looks almost good to go, with only a small required change. Can we get this ready for merging?

@maxfischer2781 maxfischer2781 self-requested a review April 14, 2022 08:07
The latest versions of the elasticsearch client have a compatibility
mode which can also be used for newer server versions; however, the other
way around does not work. Fixing the version to >=7.17,<8.0.0 is
therefore a save option for the near future.
This commit also includes a fix for the case where "resource_status" is
not part of `resource_attributes`, which occasionally caused crashes. If
this key is not present, `resource_status` will be set to an empty
string.
@stefan-k
Copy link
Contributor Author

I had this issue in the back of my head but haven't had time yet to get back to it. Thanks for the feedback, I tried to address the suggestions in the recent force push.

I guess the CI is failing due to #239.

Do you have any idea why resource_status is occassionlly not part of resource_attributes?

I was not able to figure out why, but we do see this regularly. The only suspicion I have is that it may happen with drones "resurrected" after a crash of C/T. Unfortunately our C/T does crash occasionally because monitoring plugins fail due to network issues (not sure how to fix this when considering the discussion in #189, but that's a different topic).

@stefan-k stefan-k requested a review from giffels April 14, 2022 09:27
Copy link
Member

@giffels giffels left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution. This looks fine to me.

Copy link
Member

@maxfischer2781 maxfischer2781 left a comment

Choose a reason for hiding this comment

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

Looks good to go.

@giffels giffels merged commit add98d8 into MatterMiners:master Apr 14, 2022
@stefan-k stefan-k deleted the fix_es branch April 19, 2022 05:28
giffels added a commit to giffels/tardis that referenced this pull request Apr 19, 2022
giffels added a commit to giffels/tardis that referenced this pull request Apr 19, 2022
giffels added a commit to giffels/tardis that referenced this pull request Jan 20, 2023
giffels added a commit to giffels/tardis that referenced this pull request Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Code Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants