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

Does this package still need its own implementation of Rotating/Socket/SysLog handlers? #4

Closed
jamadden opened this issue Oct 26, 2017 · 6 comments

Comments

@jamadden
Copy link
Member

It includes them, but it doesn't use them. They are completely untested.

They have been mostly untouched since they were moved here. They appear to date back to Python 2.4 or earlier (surviving log message: "remove support for Python 2.4 and 2.5"). 2.3 is when the standard library logging module was introduced. As of 2.7, the stdlib has analogues to all of these handlers. ZConfig can configure them. The default in CommonAccessLogger is to redirect logging to the Python stdlib logging package, and zope.app.server.wsgi uses that default. zope.app.server also includes ZConfig directives for configuring the Python logger this winds up using. If the other handlers are used, I'm not finding where.

I'm working on test coverage for this package, and I'd prefer not to have to write entire test suites for these things if they're not used. Ideally we'd be able to drop all of them, and perhaps simplify CommonAccessLogger to directly use the stdlib package instead of redirecting through this module's PythonLogger (with some layers to account for un/resolving of addresses). Somewhat less ideally we would be able to implement them on top of the functionality that the stdlib provides.

@mgedmin
Copy link
Member

mgedmin commented Oct 26, 2017

+1 for nuking them from orbit

A GitHub search for "zope.server.logger" finds no matches outside of zope.server (and some weird projects that have what looks like the entire zope3 repo import from before it was split into smaller independent packages).

@jamadden
Copy link
Member Author

Awesome. Should I include that in the coverage PR, do you think, or do so in a followup? Right now the coverage branch is just excluding those files from the report.

@mgedmin
Copy link
Member

mgedmin commented Oct 26, 2017

Let's do it as a separate step, afterwards.

@jamadden
Copy link
Member Author

Followup question:

The 'ResolvingLogger' wrapper for the PythonLogger wants to use an object with a resolve_ptr method that takes a callback and presumably asynchronously resolves IP addresses. The only references a github search finds to ResolvingLogger are this repository and those same copies @mgedmin found above. Further, the only credible implementation of a resolve_ptr I can find is in ZServer.

The defaults throughout this codebase and zope.app.server are to use the unresolving logger.

Given that, is it even worth keeping around the ResolvingLogger? If so, should the code that implements resolve_ptr from ZServer be brought somewhere it can be useful (i.e., this project)? Given the references in this project to "medusa" and the fact that ZServer still has packages with that name, it looks like this was originally copied/refactored from ZServer.

The code is 100% covered through mock objects, so it's not a question of code coverage anymore, just of usefulness and overhead, both cognitive and runtime, to support that abstraction.

jamadden added a commit that referenced this issue Oct 27, 2017
There may be some more cleanup depending on the decisions about resolvinglogger.

Fixes #4.
@mgedmin
Copy link
Member

mgedmin commented Oct 30, 2017

I will not miss it.

I think zope.server/zope.app.server are in pure maintenance mode, in that new applications should be using uwsgi/gunicorn/something else modern, and old applications are unlikely to start using new features.

@jamadden
Copy link
Member Author

OK. Given all that, I think I'll just not touch it. Mostly because I'm lazy.

jamadden added a commit that referenced this issue Oct 30, 2017
There may be some more cleanup depending on the decisions about resolvinglogger.

Fixes #4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants