chore(logging): configure daemon/grpc loggers on initialization #6024
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). dagit-storybook – ./js_modules/dagit/packages/core🔍 Inspect: https://vercel.com/elementl/dagit-storybook/3BaNTdsjvSn2rxZHVvgFGUb1mCuZ [Deployment for 54bea83 canceled] dagster – ./docs/next🔍 Inspect: https://vercel.com/elementl/dagster/CQViPrX2ZBnTFgbEsn1i3FeRaHuw [Deployment for 54bea83 canceled] |
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
84fa6b4
to
1801668
Compare
1801668
to
d8e062f
Compare
There was a problem hiding this comment.
would love @OwenKephart to take a look too since he's been driving the logging stuff. A few questions inline about some of the specific names
| @@ -398,7 +399,8 @@ def grpc_command( | |||
| if not (port or socket and not (port and socket)): | |||
| raise click.UsageError("You must pass one and only one of --port/-p or --socket/-s.") | |||
|
|
|||
| logger = default_system_logger("dagster-code-server", coerce_valid_log_level(log_level)) | |||
| configure_loggers(log_level=coerce_valid_log_level(log_level)) | |||
| logger = logging.getLogger("dagster.grpc.server") | |||
There was a problem hiding this comment.
I like dagster-code-server (or dagster.code-server or something) better than grpc.server personally - i've been thinking about doing a bigger push to stop calling them 'grpc servers' everywhere. (Similar to how dagit is called 'dagit' not 'the flask server' - grpc is an implementation detail)
There was a problem hiding this comment.
I think it makes sense to send the log lines to the same logger as grpc.server, since the module is currently referenced that way. While I'd be in favor of renaming the module, its probably best to put that out of scope for this PR.
That brings up another point though that specifying the logger names literally might end up leaving easy to miss bugs for if(when) we do actually refactor. We could determine the logger path programmatically with something like logger = logging.getLogger(inspect.getmodule(DagsterGrpcServer).__name__)
The effect of this is that if the import path changed, we wouldn't need to change the string as well.
d8e062f
to
11bfaeb
Compare
11bfaeb
to
ecaeb44
Compare
|
|
||
| if handler == "default": | ||
| for logger in ["dagster", "dagit"]: | ||
| logging.getLogger(logger).handlers[0].formatter.formatTime = _mockable_formatTime |
There was a problem hiding this comment.
does this need to be done here rather than in the config dictionary?
There was a problem hiding this comment.
There's a converter attribute, but unfortunately this can't be done in the config file.
ecaeb44
to
5c34710
Compare
5c34710
to
54bea83
Compare
Summary
Because we configure loggers just in time instead of on application initialization, libraries that import
system level components from Dagster (e.g. the daemon) cannot override the logging format. If one attempts
to override the logging format by introducing a new handler on the root logger, the result is that you get
double logs.
To follow best practices around logging (see https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library),
we configure the logger on application initialization. Also, we allow overrides to the logging scheme by configuring a
null handler to allow the user to use their own logging configuration.
Test Plan
run locally, see logs are still formatted properly.
Allow overrides, and see log formatting is dictated by the application code logic, rather than the library code.
pytest
Checklist