Skip to content

Refactor Logging Hierarchy in neo4j-python-driver #997

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

Merged
merged 4 commits into from
Dec 4, 2023

Conversation

OliverFarren
Copy link
Contributor

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
Copy link
Member

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?

@OliverFarren
Copy link
Contributor Author

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
Copy link
Contributor Author

@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 robsdedude self-requested a review November 29, 2023 10:58
 * Merge network and bolt child loggers
 * Rename auth child logger to auth_management
@robsdedude
Copy link
Member

robsdedude commented Nov 29, 2023

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

  • add API docs
  • merged network and bolt loggers back into one io logger (sorry for the extra work 😬). Going over the code, I found it's really hard to draw a line between raw IO (there's not much being logged there) and the bolt protocol.

Please have a look at the PR and let me know if it still looks good to you.

Copy link
Contributor Author

@OliverFarren OliverFarren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi I think this is good.

Thanks for adding the documentation. I still wonder whether there could be more granularity, but having looked through how the loggers are used in the different module i'm not sure.

If you have any thoughts i'd be interested in hearing them otherwise I think this is good

@OliverFarren
Copy link
Contributor Author

@robsdedude could we get it merged in?

Copy link
Member

@robsdedude robsdedude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📜

@robsdedude robsdedude changed the title Refactor Logging Hierarchy in neo4j-python-driver Refactor Logging Hierarchy in neo4j-python-driver Dec 4, 2023
@robsdedude robsdedude merged commit efc77a4 into neo4j:5.0 Dec 4, 2023
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

Successfully merging this pull request may close these issues.

2 participants