neo4j / neo4j-python-driver

Neo4j Bolt driver for Python
https://neo4j.com/docs/api/python-driver/current/
Other
893 stars 187 forks source link

Refactor Logging Hierarchy in neo4j-python-driver #997

Closed OliverFarren closed 9 months ago

OliverFarren commented 9 months ago

This PR includes a refactor of the logger names in the neo4j-python-driver to adhere to a hierarchical structure.

Moving to hierarchical loggers is a non-breaking changed designed to offer more control when managing logs and provides a mechanism for better organisation and filtering of logs.

import logging

logger = logging.getLogger("neo4j.async.io")
logger.setLevel(logging.CRITICAL)
robsdedude commented 9 months ago

Thank you for taking the time to craft a PR. Much appreciated.

However, before I can review the code itself, please sign the CLA (Contributor License Agreement).

General feedback from the PR description: I like the idea. However, I'd not want to leak the internal module structure. Instead, I'd prefer a logical hierarchy.

Something along the lines of

OliverFarren commented 9 months ago

Thank you for taking the time to craft a PR. Much appreciated.

However, before I can review the code itself, please sign the CLA (Contributor License Agreement).

General feedback from the PR description: I like the idea. However, I'd not want to leak the internal module structure. Instead, I'd prefer a logical hierarchy.

Something along the lines of

  • neo4j

    • .network (socket related logging)
    • .bolt (protocol messages)
    • .pool (connection pooling, and probably also routing as those two concepts are inherently coupled)
    • possibly more?

Hi @robsdedude, i've signed the CLA

That sounds good to me, so to clarify, you'd avoid the sync/async split?

OliverFarren commented 9 months ago

@robsdedude, i've consolidated the logger hierarchy, let me know your thoughts. database drivers are a bit outside my technical area so I don't have any input into the logical splitting.

robsdedude commented 9 months ago

Thanks again for the update. I went through the PR and adjusted it a little:

OliverFarren commented 9 months ago

@robsdedude could we get it merged in?