feat(server-utils): add logging configuration to server_utils to be used by servers. #13787
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## edge #13787 +/- ##
=======================================
Coverage 70.95% 70.96%
=======================================
Files 2453 2453
Lines 69023 69009 -14
Branches 8257 8257
=======================================
- Hits 48977 48971 -6
+ Misses 18094 18086 -8
Partials 1952 1952
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
My initial thought is that, if we're just going to have hard-coded configurations in here, there's little benefit over just keeping everything hard-coded in each package. I think we should either
- Set up a single, common config that applies to everything (including the full breadth of loggers/handlers)
- Have a function that can be called with a set of configurable handlers & loggers to be supplied by the caller
...or option 3 is that this isn't a useful refactor because it's a minimal amount of duplicated code to set this up independently in each server. I'll leave that up to your judgement 😄
| from .constants import * | ||
| from .logging import log_init | ||
|
|
||
| print("Im importing server_utils") |
There was a problem hiding this comment.
Inline comments, and I think I'm on Frank's side that this isn't quite a useful enough refactor yet. Specifically, it's weird to me that we have these fully qualified package names in server utils, you know? The configurations are so incredibly specific to these other servers that it's strange to have them in server-utils which is in theory a common-use library.
The thing is though that if we make this general enough that server-utils doesn't contain fully qualified package names (which I think is a good bar for "general enough"), then what does it contain on the logging front? It's already pretty terse.
I like getting config out of the opentrons package but as discussed in person I think that's only useful if we can keep opentrons from having a dependency on server_utils, which I think needs a little more refactoring before it can happen.
| from .constants import * | ||
| from .logging import log_init | ||
|
|
||
| print("Im importing server_utils") |
| @@ -140,6 +140,7 @@ async def initialize() -> ThreadManagedHardware: | |||
| """ | |||
| Initialize the Opentrons hardware returning a hardware instance. | |||
| """ | |||
| # TODO (ba, 2023-10-12): Remove log init once the opentrons_hardware process is up and running | |||
There was a problem hiding this comment.
we could remove it now if we put the logging config for the hardware controller in server utils
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| args = build_root_parser().parse_args() | ||
| systemd.configure_logging(level=args.log_level.upper()) | ||
| LOG.info(f"Starting system server on {args.host}:{args.port}") | ||
| log.info(f"Starting system server on {args.host}:{args.port}") |
There was a problem hiding this comment.
this now won't be logged, right? since the logging is set up when we invoke uvicorn now
| @@ -6,6 +6,8 @@ name = "pypi" | |||
| [packages] | |||
| aiohttp = "==3.4.4" | |||
| typing-extensions = "==3.10.0.0" | |||
| pydantic = "==1.8.2" | |||
There was a problem hiding this comment.
is this necessary? on the flex, that's a full second copy of pydantic and all its transitive deps in update-server
|
Agreed, |
Overview
We want to split up the hardware_controller from the opentrons package, we are configuring the logs for each server independently. We would like to have that configuration in a common location, so let's throw it into server-utils and some other useful variables.
Closes: RET-1381
Test Plan
Changelog
Review requests
Risk assessment
TODO