Update to a suitable version of the concurrent-log-handler#415
Conversation
We have a vendored version of ConcurrentLogHandler 0.9.1 [1] from omero-py that we use. This version is from 2013, has a few bugs, and doesn't work properly with the encoding changes [2,3] made in Python 3.10. A new version [4] whose lineage is that of the now unmaintained ConcurrentLogHandler is now available which resolves several of the aforementioned bugs, supports Python 3.10, and is drop in replacement API compatible with the previous version. This commit adds a dependency for this new version and swaps the default logger class for it in configuration. 1. https://pypi.org/project/ConcurrentLogHandler/ 2. https://peps.python.org/pep-0597/ 3. python/cpython#19481 4. https://pypi.org/project/concurrent-log-handler/
|
Assuming there are no issues with this we could probably go ahead with removing the vendored |
sbesson
left a comment
There was a problem hiding this comment.
At least conceptually, my position is that any like-for-like replacement of the forks which are currently shipped under omero_ext by upstream libraries maintained by the community is advantageous. Adding a new dependency is extremely low-cost thanks to all the infrastructure work that went into extracting and packaging omero-py/omero-web and allows us to benefit from several features including security detection etc.
Overall no objection from my side but a few general comments:
- based on a similar attempt with argparse, I would not exclude we have been relying on some API feature that is only present in our vendored version so I would definitely give this PR a few runs in the nightly CI builds
- in terms of functional testing, we probably want to confirm the log rotation works as expected. This might be best achieved by temporary enabling
omero.web.debug=Trueon a deployment as this has the best potential of generating GBs of logs - while looking at this code path recently, I was wondering how easy it would to add extra configuration for the log rotation (typically log size and/or number of concurrent logs) Since all the
omero.web.*properties are defined insettings.pyafter the logging initialization, this seems a bit more involved that just adding extra properties. - I also vote for the long-term removal of
omero_ext.cloghandlerfromomero-pybut I would probably argue for a more traditional lifecycle of deprecation in 5.x.x and removal in 6.0.0 in case someone depends on it
|
Diff between our vendored version and the original 0.9.1 for anyone concerned about specific semantics we've added that we may be relying upon: |
joshmoore
left a comment
There was a problem hiding this comment.
MSTM. Since there aren't tests specifically on log rolling with the production settings (for good reason), we might deploy somewhere and trigger a rollover. The merge-ci web doesn't have enough logging at the moment to do so, but then again neither does Processor or co. (Did you consider the same PR for omero-py, @chris-allan?)
I wasn't even aware we're using it in |
No, glancing quickly, you're probably right. I very much assumed we were using the same infra both the Python services as well. |
|
Changes included in 5.16.0 |
Thanks @jburel |
We have a vendored version of ConcurrentLogHandler 0.9.1 1 from omero-py that we use. This version is from 2013, has a few bugs, and doesn't work properly with the encoding changes 23 made in Python 3.10.
A new version 4 whose lineage is that of the now unmaintained ConcurrentLogHandler is now available which resolves several of the aforementioned bugs, supports Python 3.10, and is drop in replacement API compatible with the previous version. This commit adds a dependency for this new version and swaps the default logger class for it in configuration.
Footnotes
https://pypi.org/project/ConcurrentLogHandler/ ↩
https://peps.python.org/pep-0597/ ↩
https://github.com/python/cpython/pull/19481 ↩
https://pypi.org/project/concurrent-log-handler/ ↩