chore(log): use loguru instead of logging #713
Conversation
| @@ -42,7 +42,6 @@ To run the tests: | |||
|
|
|||
| ### Environment Variables | |||
| - `WREN_ENGINE_ENDPOINT`: The endpoint of the Wren Java engine | |||
| - `LOG_LEVEL`: The log level of the server (default is INFO) | |||
There was a problem hiding this comment.
Curiously, it seems that loguru doesn't use this env. How can we change the log level after moving to loguru?
There was a problem hiding this comment.
The main principle of loguru is that there are no levels. By default, everything is printed to sys.stderr. It then allows you to choose to output specific levels to designated files. Please refer to Delgan/loguru#138 (comment).
BTW, the default level is DEBUG.
There was a problem hiding this comment.
So, everything is logged to sys.stderr. IMO, sys.stderr is not always printed in the console. Maybe we should check whether the environment in the docker container will print it or not.
There was a problem hiding this comment.
After testing, the log will be printed in the console.
grieve54706
force-pushed
the
feature/config-edit
branch
2 times, most recently
from
424e66e to
911b87b
Compare
| return ( | ||
| rewritten_sql | ||
| if self.data_source is None | ||
| else self.transpile(rewritten_sql) | ||
| ) | ||
| except httpx.ConnectError as e: | ||
| raise ConnectionError(f"Can not connect to Wren Engine: {e}") | ||
| except httpx.HTTPStatusError as e: | ||
| raise RewriteError(e.response.text) |
There was a problem hiding this comment.
I think we can also wrap the rewrite exception into RewriteError for EmbeddedEngineRewriter, then I guess we can be powered by the exception handler in v3/connector.py to generate the response.
ibis-server/app/main.py
Outdated
| @app.patch("/config") | ||
| def update_config(diagnose: bool): | ||
| config = get_config() | ||
| config.update(diagnose=diagnose) | ||
| return config |
There was a problem hiding this comment.
How about adding a test to check if this update works?
grieve54706
force-pushed
the
feature/config-edit
branch
from
911b87b to
541f248
Compare
There was a problem hiding this comment.
Thanks @grieve54706 Looks nice to me. Just one question about config updating.
| async def query_dry_run_error_handler(request, exc: QueryDryRunError): | ||
| return PlainTextResponse(str(exc), status_code=422) | ||
| @app.patch("/config") | ||
| def update_config(config_model: ConfigModel): |
There was a problem hiding this comment.
I saw you get_config().init_logger at L17. Should we init the logger again after updating the config?
There was a problem hiding this comment.
We will remove the old logger and add a new logger when the config is updated in the config.py.
| def transpile(self, planned_sql: str) -> str: | ||
| dialect_sql = sqlglot.transpile( | ||
| planned_sql, read="trino", write=self.data_source.name |
Use loguru instead of logging.
It could provide more features, such as associating request logs with a UUID, which is particularly helpful in multi-threaded environments.
Additionally, it allows the entire stack trace to be displayed, including the values of variables.
Add API to control diagnose
Loguru allows log values of variables when
diagnoseis True. Ref fully-descriptive-exceptionsRemove log level
The main principle of
loguruis that there are no levels. By default, everything is printed tosys.stderr. It then allows you to choose to output specific levels to designated files. Please refer to Delgan/loguru#138 (comment)Use middleware instead of log_dto
By using middleware, we can more elegantly print out the request's parameters and body, and have their logs associated with a UUID. But the request body is not deserialized to the Pydantic model, we need to deserialize the body to JSON and then mask connection information.
Pass Java engine response
Previously, we would only return the
Server Internal Errormessage that occurred when ibis-server called the Java engine to the caller. Now, we will return the error message from the Java engine itself.Rename variable
Make the rewritten sql with the manifest name
planned_sql.Make the transpired sql by sqlglot name
dialect_sql.Minor