-
Notifications
You must be signed in to change notification settings - Fork 45
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
Improve logging #235
base: main
Are you sure you want to change the base?
Improve logging #235
Changes from all commits
e4342e8
3033ef8
80708e4
37f964a
67174eb
741a103
2f47d00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
# Copyright (c) "Neo4j" | ||
# Neo4j Sweden AB [https://neo4j.com] | ||
# # | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# # | ||
# https://www.apache.org/licenses/LICENSE-2.0 | ||
# # | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
from __future__ import annotations | ||
|
||
import os | ||
from typing import Any | ||
|
||
from pydantic import BaseModel | ||
|
||
DEFAULT_MAX_LIST_LENGTH: int = 5 | ||
DEFAULT_MAX_STRING_LENGTH: int = 200 | ||
|
||
|
||
class Prettyfier: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have any unit tests for this class? I think testing the nested functionality would be especially useful |
||
"""Prettyfy any object for logging. | ||
|
||
I.e.: truncate long lists and strings, even nested. | ||
|
||
Max list and string length can be configured using env variables: | ||
- LOGGING__MAX_LIST_LENGTH (int) | ||
- LOGGING__MAX_STRING_LENGTH (int) | ||
""" | ||
|
||
def __init__(self) -> None: | ||
self.max_list_length = int( | ||
os.environ.get("LOGGING__MAX_LIST_LENGTH", DEFAULT_MAX_LIST_LENGTH) | ||
) | ||
self.max_string_length = int( | ||
os.environ.get("LOGGING__MAX_STRING_LENGTH", DEFAULT_MAX_STRING_LENGTH) | ||
) | ||
|
||
def _prettyfy_dict(self, value: dict[Any, Any]) -> dict[Any, Any]: | ||
return { | ||
k: self(v) # prettyfy each value | ||
for k, v in value.items() | ||
} | ||
|
||
def _prettyfy_list(self, value: list[Any]) -> list[Any]: | ||
items = [ | ||
self(v) # prettify each item | ||
for v in value[: self.max_list_length] | ||
] | ||
remaining_items = len(value) - len(items) | ||
if remaining_items > 0: | ||
items.append(f"... ({remaining_items} items)") | ||
return items | ||
|
||
def _prettyfy_str(self, value: str) -> str: | ||
new_value = value[: self.max_string_length] | ||
remaining_chars = len(value) - len(new_value) | ||
if remaining_chars > 0: | ||
new_value += f"... ({remaining_chars} chars)" | ||
return new_value | ||
|
||
def __call__(self, value: Any) -> Any: | ||
"""Takes any value and returns a prettified version for logging.""" | ||
if isinstance(value, dict): | ||
return self._prettyfy_dict(value) | ||
if isinstance(value, BaseModel): | ||
return self(value.model_dump()) | ||
if isinstance(value, list): | ||
return self._prettyfy_list(value) | ||
if isinstance(value, str): | ||
return self._prettyfy_str(value) | ||
return value | ||
|
||
|
||
prettyfier = Prettyfier() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why instantiate an instance of the Prettyfier class here and then import the instance? Why not just import the class and then create an instance of it where it's used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main reason is to keep things simple when logging, we already need an extra step (calling the function), if we also have to instantiate the class every time we use a logger, I think it makes the code more complicated. We could also have a function like this:
and import this function when logging, but since the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy to leave it for now as I don't have a good intuition for what would be the best approach in this situation. It might be something worth changing in a future refactor though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor but this should probably be
instantiating
and notinstantiate