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

feat: replace custom reporter package with slog #1666

Merged
merged 37 commits into from
Mar 5, 2025

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Feb 26, 2025

Our custom reporter package has served us well, but it's annoying to have to pass it through to every function especially when you're using the libraries, so this switches us over to using slog which uses the global default logger - the notable downside with this is as a global tests that depend on logging output cannot be run in parallel, but that shouldn't be a big deal for downstream consumers as they should be depending on the structured data returned by package functions (i.e. our packages only log information that could be useful for a human to debug stuff).

This downside does impact our CLI test suite since that currently does depend on that logging output as part of feedback - for now I've just "fixed" this by removing t.Parallel from our CLI tests, which makes them take a lot longer but should make this landable straightaway, after which we'll be free to chop and change what we like since we'll have gotten rid of the public reporter package and log output is not covered by semver.

I've already got a few ideas following on from this on how we might be able to speed our CLI tests back up, starting with attempting to chop the tests up into actual small packages, since Go supports running package tests in parallel regardless of their dependency on globals.


I do still need to give this a manual review, but I believe the primary thing left before this could be landed (warts or not) is to ensure when we're outputting JSON all other output goes to stderr

@G-Rath G-Rath force-pushed the output/use-slog branch 5 times, most recently from d0371e6 to 7c7815a Compare February 26, 2025 19:36
@G-Rath G-Rath force-pushed the output/use-slog branch 3 times, most recently from f58ee94 to 2ea5642 Compare February 27, 2025 02:15
@G-Rath G-Rath force-pushed the output/use-slog branch 5 times, most recently from 2d48459 to e752f24 Compare February 27, 2025 21:52
@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2025

Codecov Report

Attention: Patch coverage is 67.59259% with 140 lines in your changes missing coverage. Please review.

Project coverage is 64.58%. Comparing base (8a7e9d9) to head (69c6d46).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
cmd/osv-scanner/fix/noninteractive.go 37.50% 12 Missing and 3 partials ⚠️
cmd/osv-reporter/main.go 0.00% 14 Missing ⚠️
cmd/osv-scanner/fix/main.go 54.16% 11 Missing ⚠️
cmd/osv-scanner/internal/helper/helper.go 62.06% 11 Missing ⚠️
cmd/osv-scanner/update/main.go 28.57% 10 Missing ⚠️
internal/sourceanalysis/rust.go 20.00% 8 Missing ⚠️
pkg/osvscanner/osvscanner.go 77.77% 8 Missing ⚠️
cmd/osv-scanner/fix/output.go 66.66% 7 Missing ⚠️
cmd/osv-scanner/scan/image/main.go 46.15% 6 Missing and 1 partial ⚠️
cmd/osv-scanner/scan/source/main.go 50.00% 5 Missing and 1 partial ⚠️
... and 15 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1666      +/-   ##
==========================================
+ Coverage   64.55%   64.58%   +0.03%     
==========================================
  Files         156      157       +1     
  Lines       15981    15840     -141     
==========================================
- Hits        10316    10230      -86     
+ Misses       4983     4932      -51     
+ Partials      682      678       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@G-Rath G-Rath force-pushed the output/use-slog branch 7 times, most recently from 5ffddc9 to fafe77d Compare March 3, 2025 00:31
@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 3, 2025

... and there we go: completely moved off the public reporter package without any changes to the snapshots or cli behaviour 🎉

There are definitely ways that this can (and should) be improved which I've already got some ideas for that I'll start working on next, but I think I'll do all of them as follow-ups since this should be the primarily (and hopefully only) breaking change that needs to be included in v2 specifically, and this PR is already very big.

Currently on my list:

  • review newline handling (though I think actually we should maybe leave newlines to the caller, otherwise we'll always have to ensure everything is single-lined...)
  • review logs in regard to having their level manually prefixed (we shouldn't do that I think in most cases)
  • review if it's appropriate to be using slog in cmd packages (it might be a bit nicer in some of those to just output?)
  • cleanup the clilogger "global" functions (I'd like to avoid the multiple doubled up functions)
  • cleanup internal/reporter/verbosity.go (we don't need the aliased type now)
  • consider renaming and merging clilogger, output, and reporter packages
    • I think they could be a single package now? though output is quite large, but reporter can definitely be merged with one of the other two
    • we could prefix all three with cli or cmd to group them, or maybe it'd be enough to just move them to cmd/internal?
  • (otherwise) see if we can avoid having to access clilogger.HasErrored in output package
  • look into switching the library packages into outputting actual structured logs, and then having the CLI side of things handle turning said structured records into "human" output lines
  • consider switching from "cli" to "cmd" (to match root level namespace), and likewise moving package into cmd/internal or similar
  • consider adding a verbosity flag to the other commands
  • consider if we want to introduce our own internal logger package, similar to osv-scalibr (it might be useful for managing our output vs external package output - by default, its global logger would be global slog one)

@another-rex let me know if you've got any thoughts on the above, or have anything to add to that list

(oh and of course there's the whole "disabling of t.Parallel thing...)

@G-Rath

This comment was marked as outdated.

@G-Rath G-Rath force-pushed the output/use-slog branch from fafe77d to 7c1b978 Compare March 3, 2025 02:13
@G-Rath G-Rath marked this pull request as ready for review March 3, 2025 02:46
@G-Rath G-Rath force-pushed the output/use-slog branch from 7c1b978 to 03da028 Compare March 3, 2025 03:17
Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

Looking great! It looks like the tests "only" took 4 minutes on ubuntu? That's surprisingly fast all things considered.

Verified

This commit was signed with the committer’s verified signature.
mpalmi Mike Palmiotto
G-Rath added 21 commits March 4, 2025 08:49

Verified

This commit was signed with the committer’s verified signature.
mpalmi Mike Palmiotto

Verified

This commit was signed with the committer’s verified signature.
mpalmi Mike Palmiotto

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@G-Rath G-Rath force-pushed the output/use-slog branch from 03da028 to 0dc60fa Compare March 3, 2025 19:49
@another-rex another-rex merged commit bb40b20 into google:main Mar 5, 2025
13 checks passed
@another-rex another-rex deleted the output/use-slog branch March 5, 2025 00:23
@spencerschrock
Copy link
Contributor

Browsing patch notes as I make the migration to v2 and saw this PR.

the notable downside with this is as a global tests that depend on logging output cannot be run in parallel

FYI @G-Rath, a proposal (marked as likely accept) could help with this in a future Go version.

@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 9, 2025

@spencerschrock unfortunately I don't think that proposal will help - if I understand correctly, it's about the output from tests being printed which can get messy especially when tests are run in parallel, whereas our issue is that we're using a global and t.Parallel works by using goroutines

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.

None yet

4 participants