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

Implements #11 - JSONL logging instead of printing to stdout #106

Merged
merged 26 commits into from
Oct 21, 2024

Conversation

dario-br
Copy link
Contributor

Added logging capabilities to JSONL.

Caveat: still missing the result/status of the parser/analyser when logging.

… present:

1. added logger at module level
2. replace print statement by correspoding logging statement: info, warning, error and debug (for commented prints)
1. another demo parser??
2. get_result method when using cached results
…onsole only as per cmd parameter (configured by default to WARNING).

File logging rules:
- JSONL format. Note: Still breaks if double quotes added to the message.
- File per execution (analyse, parse), per case.
1. Use of proper JSON formatter
2. Use of extra and exc_info fields when logging. The usage of extra fields needs to be standardised
@dario-br dario-br requested a review from cvandeplas October 15, 2024 10:51
@dario-br dario-br linked an issue Oct 15, 2024 that may be closed by this pull request
@cvandeplas
Copy link
Contributor

Cool !

I notice there is quite some redundant code in all the parser/analyser files.
Would it be an option to define the logger in the BaseInterface? That way every module can call it as self.logger.info() without the need to redefine the logger = logging.getLogger('sysdiagnose')

@dario-br
Copy link
Contributor Author

dario-br commented Oct 15, 2024

it would, if we would not use static methods :) because that was my first approach... and I don't dare to go for such code revamp

@cvandeplas
Copy link
Contributor

We can drop the static methods, that would be cleaner, no?

@dario-br
Copy link
Contributor Author

I have got another idea, so that we do not even drop the static methods.

I move all the initialisation code to utils/logger.py (instead of jsonlogger.py as it is now) and in the rest of the files, we just do an import from that logger.py

from sysdiagnose.utils.logger import logger

Where logger,

logger = blablablabla

That would also be a clean option, wouldn't it? What do you think?

https://stackoverflow.com/questions/50391429/logging-in-classes-python

src/sysdiagnose/main.py Outdated Show resolved Hide resolved
for case_id in case_ids:
# Handle file logging
time_str = time.strftime("%Y%m%dT%H%M%S")
filename = f"{time_str}-parse-{case_id}.jsonl"
Copy link
Contributor

Choose a reason for hiding this comment

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

The file is saved in the parsed_data_folder. Is there real value in adding the case_id in the filename?
I would propose to use something like log-{time_str}-parse.jsonl instead or {time_str}-log-parse.jsonl.
Second question: is it appending logs to that file? So if a user runs parser X, and later on parser Y the logs are appended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you run parse/analyse all, all logs are appended to the same file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 62354a5

…parameter resides. That is, available to all modes
@cvandeplas cvandeplas merged commit b16133e into main Oct 21, 2024
5 checks passed
@cvandeplas cvandeplas deleted the issue-11-logging branch October 21, 2024 07:02
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.

(enhancement): use python's logger instead of prints
2 participants