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

Use structured logging #105

Merged
merged 55 commits into from
Jan 25, 2025
Merged

Use structured logging #105

merged 55 commits into from
Jan 25, 2025

Conversation

blakejameson
Copy link
Contributor

@blakejameson blakejameson commented Jan 21, 2025

Implemented the beginning stages of a Logger class

The logger is used in replace of the debug_print and print statements.

Regarding the error_print statements, I have currently refactored the error_print function to use the logger internally. The error_print has an increment to self.c_error_count, so refactoring wasn't a simple switch to the logger error message and required some thought to approach a refactor. I am following Nate's guidance to refactor this section after his and Taylor's merge to the change counters (#93 #95).

This logger class currently has the functionality to format key, value pairs into a json string, which can be printed to standard output.

Currently, there IS NOT the ability to save these logs to a file. This functionality is a longer-term objective.

I tested this code by running it on a v4b board, and the output shows similar to the main branch, the only difference of course seeing the output in json

@blakejameson blakejameson requested a review from a team January 21, 2025 23:54
@blakejameson blakejameson linked an issue Jan 21, 2025 that may be closed by this pull request
@blakejameson blakejameson force-pushed the use-structured-logging branch from 5d920e7 to fd44b97 Compare January 22, 2025 00:48
@blakejameson
Copy link
Contributor Author

NOTE:
Recently rebased onto main after the hardware init PR was merged in.

Resolved merge conflicts, but am currently running into an error and board ends up crashing. Need to explore and mitigate the issue

blakejameson and others added 26 commits January 21, 2025 19:50
…n errors but managed to get past them with the work done here. A lot more work to be done
…hat will later be removed as I decide on one of the formats. still very early on in process
…ew function calls to the log functions in main.py
…logger from functions.py can be passed into the cdh functions and that logger can be used.
…modified debug_print so it ends up calling the logger.
…rameter. changed debug_print function calls to logger function calls
…call parameters in time since I added this line of code
lib/requirements.txt Outdated Show resolved Hide resolved
@blakejameson
Copy link
Contributor Author

This was a lot of work and it's almost in! I like the logger interface that you created, that looks nice and easy to use. I noticed some repeated code in the logger implementation. What do you think of this? #109?

I know the old logger in this repo tracked file names and I played around with the concept of tracking filenames with the interface you created for this logger but I'm just not won over. Log messages are usually unique enough to help track down issues, tracking files adds boiler plate filename= vars at the top of every file that wants to use it, and I don't see this kind of tracking in other loggers so what about nixing it all together?

Thank you for the feedback in #109

Regarding the filenames, I am with you. It didn't feel quite right regarding the 'filename=' at the top of different files and felt kinda clumsy or awkward. I am definitely supportive of getting rid of the filename parameter

Copy link
Member

@nateinaction nateinaction left a comment

Choose a reason for hiding this comment

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

You made quick work of that review yesterday! Awesome! I've left a few comments and suggestions on calls to the logger that could be implemented in more places the themes for the call outs are:

  • Remove the message= key from the first argument in each logger call
  • Ensure that every call to the logger has a non-empty message
  • Move all variables injected into message strings to use key/value pairs (kwargs) that you feed to the logger
  • Add tests for all of the public functions you've created in logger.py

Because of the conflicts with this large change, I want to make sure this is the next thing that goes in if we can so thank you for the quick follow up.

What do you think of the proposed changes?

lib/pysquared/battery_helper.py Outdated Show resolved Hide resolved
lib/pysquared/battery_helper.py Outdated Show resolved Hide resolved
lib/pysquared/battery_helper.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
@@ -4,42 +4,38 @@
Authors: Nicole Maggard, Michael Pham, and Rachel Sarmiento
"""

import traceback
Copy link
Member

Choose a reason for hiding this comment

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

Noting for the future that we talked about removing traceback and we couldn't find a reason to keep it. We tried both in repl and found that reporting back the exception var e gave the same output but maybe we missed something. 🤷

Copy link
Member

@nateinaction nateinaction left a comment

Choose a reason for hiding this comment

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

Had a few final comments. What are your thoughts?

Not directed at you or this PR but oof @ that big try/except block in cdh.py. Cleaning that up could make logging nicer but that makes sense to do in another PR.

lib/pysquared/Field.py Outdated Show resolved Hide resolved
lib/pysquared/Field.py Outdated Show resolved Hide resolved
lib/pysquared/cdh.py Outdated Show resolved Hide resolved
lib/pysquared/cdh.py Outdated Show resolved Hide resolved
lib/pysquared/cdh.py Outdated Show resolved Hide resolved
lib/pysquared/packet_manager.py Outdated Show resolved Hide resolved
lib/pysquared/packet_manager.py Outdated Show resolved Hide resolved
lib/pysquared/packet_sender.py Outdated Show resolved Hide resolved
lib/pysquared/logger.py Outdated Show resolved Hide resolved
lib/pysquared/logger.py Outdated Show resolved Hide resolved
Copy link
Member

@nateinaction nateinaction left a comment

Choose a reason for hiding this comment

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

image

Great job!

@blakejameson blakejameson merged commit 04fcbb9 into main Jan 25, 2025
3 checks passed
@blakejameson blakejameson deleted the use-structured-logging branch January 25, 2025 06:05
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.

Use structured logging
2 participants