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

chore: set logger level to error to reduce noise from Elasticsearch and OpenSearch client libraries #4979

Merged

Conversation

jfcalvo
Copy link
Member

@jfcalvo jfcalvo commented Jun 7, 2024

Description

This PR includes some changes to reduce the logger level of Elasticsearch and OpenSearch to error. With this we are reducing a little bit the output noise of these client libraries.

I have improved a little bit the settings so only elasticsearch and opensearch are valid options for search_engine attribute. I have added some small useful functions too.

Refs #4946

Type of change

(Please delete options that are not relevant. Remember to title the PR according to the type of change)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (change restructuring the codebase without changing functionality)
  • Improvement (change adding some improvement to an existing functionality)
  • Documentation update

How Has This Been Tested

(Please describe the tests that you ran to verify your changes. And ideally, reference tests)

  • Manually testing that warning messages are not showed anymore using Elasticsearch and OpenSearch.

Checklist

  • I added relevant documentation
  • follows the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I filled out the contributor form (see text above)
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@jfcalvo jfcalvo requested a review from frascuchon June 7, 2024 15:05
Copy link

codecov bot commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 62.96296% with 10 lines in your changes missing coverage. Please review.

Project coverage is 91.18%. Comparing base (d1bfec6) to head (5654e50).
Report is 7 commits behind head on feat/v2.0.0.

Files Patch % Lines
argilla-server/src/argilla_server/_app.py 42.85% 8 Missing ⚠️
argilla-server/src/argilla_server/settings.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           feat/v2.0.0    #4979       +/-   ##
================================================
+ Coverage        60.92%   91.18%   +30.26%     
================================================
  Files              329      136      -193     
  Lines            17674     5855    -11819     
================================================
- Hits             10767     5339     -5428     
+ Misses            6907      516     -6391     
Flag Coverage Δ
argilla ?
argilla-server 91.18% <62.96%> (-0.14%) ⬇️

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.

Comment on lines 102 to 105
search_engine: Union[
Literal[SEARCH_ENGINE_ELASTICSEARCH],
Literal[SEARCH_ENGINE_OPENSEARCH],
] = SEARCH_ENGINE_ELASTICSEARCH
Copy link
Member

@frascuchon frascuchon Jun 10, 2024

Choose a reason for hiding this comment

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

Even if we can define constants for official engines, we shouldn't fix the available options.

Creating new custom engine or specific engine connection could be helpful for the community but, by fixing this we’re avoiding this possibility. (See how engine are registered)

Copy link
Member Author

@jfcalvo jfcalvo Jun 10, 2024

Choose a reason for hiding this comment

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

But in the case that a PR from the community adds a new engine they should modify this Union as part of the PR right? I don't get the problem.

(See how engine are registered)

I missed that one. I will use the constant there too.

return self.search_engine == SEARCH_ENGINE_OPENSEARCH

@property
def humanized_search_engine(self) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

The name of this method gives me chills...

"I've seen things you people wouldn't believe. Attack ships on fire off the shoulder of Orion. I watched C-beams glitter in the dark near the Tannhäuser Gate. All those moments will be lost in time like tears in rain..."

Could we just call human_readable_search_engine? And also generalize a bit by using str.title() ? I think we don't need al this extra functions if we keep simple

Copy link
Member Author

@jfcalvo jfcalvo Jun 10, 2024

Choose a reason for hiding this comment

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

"Humanize" is a very common concept and naming when formatting strings but I don't have any problem changing it to "human_readable_*". 🙂

Some examples:

@jfcalvo jfcalvo requested a review from frascuchon June 12, 2024 11:20
@jfcalvo
Copy link
Member Author

jfcalvo commented Jun 12, 2024

@frascuchon I applied feedback from our discussion:

  • Removed the humanized_search_engine property from settings.
  • Removed the Union restricting the search_engine only to elasticsearch or opensearch values.

@jfcalvo jfcalvo merged commit 560bd7a into feat/v2.0.0 Jun 12, 2024
8 checks passed
@jfcalvo jfcalvo deleted the chore/set-logger-level-to-error-for-search-engine branch June 12, 2024 14:12
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.

2 participants