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

Add optional JSON logging #83

Merged
merged 3 commits into from
Oct 19, 2023
Merged

Add optional JSON logging #83

merged 3 commits into from
Oct 19, 2023

Conversation

drdavella
Copy link
Member

@drdavella drdavella commented Oct 18, 2023

Overview

Implement JSON logging per the logging spec

{"timestamp": "2023-10-18 14:36:18,122", "level": "INFO", "message": "\n[startup]", "file": "logging.py", "line": 46}
{"timestamp": "2023-10-18 14:36:18,122", "level": "INFO", "message": "codemodder: python/0.57.0", "file": "codemodder.py", "line": 135}
{"timestamp": "2023-10-18 14:36:18,122", "level": "INFO", "message": "\n[setup]", "file": "logging.py", "line": 46}
{"timestamp": "2023-10-18 14:36:18,122", "level": "INFO", "message": "running:", "file": "logging.py", "line": 53}

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #83 (fd95622) into main (9aa46b3) will decrease coverage by 0.38%.
The diff coverage is 74.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
- Coverage   96.31%   95.94%   -0.38%     
==========================================
  Files          48       48              
  Lines        1874     1900      +26     
==========================================
+ Hits         1805     1823      +18     
- Misses         69       77       +8     
Files Coverage Δ
src/codemodder/cli.py 100.00% <100.00%> (ø)
src/codemodder/codemodder.py 98.11% <100.00%> (ø)
src/codemodder/logging.py 82.60% <72.41%> (-17.40%) ⬇️

@drdavella drdavella force-pushed the json-logging branch 3 times, most recently from 45e2b3b to f04add6 Compare October 18, 2023 18:37
@drdavella drdavella marked this pull request as ready for review October 18, 2023 18:56
@drdavella
Copy link
Member Author

I don't really understand what's happening here, when I run this locally it's telling me logging.py is even more covered than it was before:

src/codemodder/logging.py                                             46      8    83%

self.project_name = project_name
super().__init__(*args, **kwargs)

def add_fields(self, log_record, record, message_dict):
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm tempted to ignore this block for the purposes of coverage. I tried to write some tests here but it's proving fairly difficult to get the mocks right.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm super surprised codecov says most of this isn't tested. Curious, if you put a debugger in here and run codemodder as pixeebot would, it does it it, right? That would tell me integration tests would test this but right now we only calc coverage based on unit tests. This makes sense bc the way we run integration tests with subprocess.run creates a separate process and pytest / cov can't calculate coverage there.

Is there any help from the python-json-logger docs in regards to testing? Does chatgpt help in any way here? If you've tried these avenues and felt like you've spent sufficient time, I would be ok with adding no-cover here though it does break my heart a little.

Copy link
Member Author

Choose a reason for hiding this comment

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

This codepath is only covered if we use --log-format=json which probably doesn't happen in any of the integration tests right now. This basically just boils down to level of effort vs reward right now and I can't afford to spend any more time trying to get this covered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally fine, ticket and move on. Could be a good starter task for someone who may join in the upcoming months or just an easy "end of day" task.


stderr_handler = logging.StreamHandler(sys.stderr)
stderr_handler.setLevel(logging.ERROR)
match log_format:
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

@drdavella drdavella merged commit 2ad1030 into main Oct 19, 2023
10 of 11 checks passed
@drdavella drdavella deleted the json-logging branch October 19, 2023 16:16
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.

3 participants