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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions argilla-server/src/argilla_server/_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
for app_configure in [
configure_app_logging,
configure_database,
ping_search_engine,
configure_search_engine,
configure_telemetry,
configure_middleware,
configure_app_security,
Expand Down Expand Up @@ -148,18 +148,35 @@
)


def ping_search_engine(app: FastAPI):
def configure_search_engine(app: FastAPI):
@app.on_event("startup")
async def configure_elasticsearch():
if not settings.search_engine_is_elasticsearch:
return

Check warning on line 155 in argilla-server/src/argilla_server/_app.py

View check run for this annotation

Codecov / codecov/patch

argilla-server/src/argilla_server/_app.py#L154-L155

Added lines #L154 - L155 were not covered by tests

logging.getLogger("elasticsearch").setLevel(logging.ERROR)
logging.getLogger("elastic_transport").setLevel(logging.ERROR)

Check warning on line 158 in argilla-server/src/argilla_server/_app.py

View check run for this annotation

Codecov / codecov/patch

argilla-server/src/argilla_server/_app.py#L157-L158

Added lines #L157 - L158 were not covered by tests

@app.on_event("startup")
async def configure_opensearch():
if not settings.search_engine_is_opensearch:
return

Check warning on line 163 in argilla-server/src/argilla_server/_app.py

View check run for this annotation

Codecov / codecov/patch

argilla-server/src/argilla_server/_app.py#L162-L163

Added lines #L162 - L163 were not covered by tests

logging.getLogger("opensearch").setLevel(logging.ERROR)
logging.getLogger("opensearch_transport").setLevel(logging.ERROR)

Check warning on line 166 in argilla-server/src/argilla_server/_app.py

View check run for this annotation

Codecov / codecov/patch

argilla-server/src/argilla_server/_app.py#L165-L166

Added lines #L165 - L166 were not covered by tests

@app.on_event("startup")
@backoff.on_exception(backoff.expo, ConnectionError, max_time=60)
async def _ping_search_engine():
async def ping_search_engine():
async for search_engine in get_search_engine():
if not await search_engine.ping():
raise ConnectionError(
f"Your Elasticsearch endpoint at {settings.obfuscated_elasticsearch()} is not available or not responding.\n"
"Please make sure your Elasticsearch instance is launched and correctly running and\n"
f"Your {settings.humanized_search_engine} endpoint at {settings.obfuscated_elasticsearch()} is not available or not responding.\n"
f"Please make sure your {settings.humanized_search_engine} instance is launched and correctly running and\n"
"you have the necessary access permissions. Once you have verified this, restart the argilla server.\n"
)


def configure_app_security(app: FastAPI):
auth.configure_app(app)

Expand Down
3 changes: 3 additions & 0 deletions argilla-server/src/argilla_server/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
API_KEY_HEADER_NAME = "X-Argilla-Api-Key"
WORKSPACE_HEADER_NAME = "X-Argilla-Workspace"

SEARCH_ENGINE_ELASTICSEARCH = "elasticsearch"
SEARCH_ENGINE_OPENSEARCH = "opensearch"

DEFAULT_USERNAME = "argilla"
DEFAULT_PASSWORD = "1234"
DEFAULT_API_KEY = "argilla.apikey"
Expand Down
24 changes: 22 additions & 2 deletions argilla-server/src/argilla_server/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,16 @@
import re
import warnings
from pathlib import Path
from typing import List, Optional
from typing import List, Literal, Optional, Union
from urllib.parse import urlparse

from argilla_server.constants import (
DEFAULT_LABEL_SELECTION_OPTIONS_MAX_ITEMS,
DEFAULT_MAX_KEYWORD_LENGTH,
DEFAULT_SPAN_OPTIONS_MAX_ITEMS,
DEFAULT_TELEMETRY_KEY,
SEARCH_ENGINE_ELASTICSEARCH,
SEARCH_ENGINE_OPENSEARCH,
)
from argilla_server.pydantic_v1 import BaseSettings, Field, root_validator, validator

Expand Down Expand Up @@ -97,7 +99,10 @@

es_mapping_total_fields_limit: int = 2000

search_engine: str = "elasticsearch"
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.


vectors_fields_limit: int = Field(
default=5,
Expand Down Expand Up @@ -217,6 +222,21 @@
return index_name.replace("<NAMESPACE>", "")
return index_name.replace("<NAMESPACE>", f".{ns}")

@property
def search_engine_is_elasticsearch(self) -> bool:
return self.search_engine == SEARCH_ENGINE_ELASTICSEARCH

Check warning on line 227 in argilla-server/src/argilla_server/settings.py

View check run for this annotation

Codecov / codecov/patch

argilla-server/src/argilla_server/settings.py#L227

Added line #L227 was not covered by tests

@property
def search_engine_is_opensearch(self) -> bool:
return self.search_engine == SEARCH_ENGINE_OPENSEARCH

Check warning on line 231 in argilla-server/src/argilla_server/settings.py

View check run for this annotation

Codecov / codecov/patch

argilla-server/src/argilla_server/settings.py#L231

Added line #L231 was not covered by tests

@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:

if self.search_engine_is_elasticsearch:
return "Elasticsearch"
elif self.search_engine_is_opensearch:
return "OpenSearch"

Check warning on line 238 in argilla-server/src/argilla_server/settings.py

View check run for this annotation

Codecov / codecov/patch

argilla-server/src/argilla_server/settings.py#L235-L238

Added lines #L235 - L238 were not covered by tests

def obfuscated_elasticsearch(self) -> str:
"""Returns configured elasticsearch url obfuscating the provided password, if any"""
parsed = urlparse(self.elasticsearch)
Expand Down
Loading