⚠️ log: Initial `logr`/`logrusr` implementation #1516

justaugustus · 2022-01-23T11:36:55Z

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes / features): testing: Add unit tests for log package #1528
PR title follows the guidelines defined in https://github.com/ossf/scorecard/blob/main/CONTRIBUTING.md#pr-process

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Continues #1502.
ref: #1273

Signed-off-by: Stephen Augustus [email protected]

What is the current behavior? (You can also link to an open issue here)

#1502 implemented a generic logger that wrapped zap. Create a new logger is done by calling log.Logger(log.Level).

What is the new behavior (if this is a feature change)?

log.Logger now uses logr, which provides a sane interface and has multiple popular implementations.

From @thockin in #1273 (comment):

Stephen pinged me for a consult, so here I am :)

Opinion: About 1% of apps can actually justify writing logging abstractions, but 90% of us end up doing it because there isn't a (literal or de facto) standard.

I offer you http://github.com/go-logr/logr as that standard. logr defines an API, but not an impl. There are many implementations available (including glog, klog, zap, logrus, and zerolog). Your main package can have full knowledge of the impl, set it up as it likes, and then pass it down to the rest of the app.

EVERYTHING ELSE only needs to know the logr API. The logr package has no external dependencies (and will not grow them, scout's honor), so it is a "safe" thing to depend on. And it's TINY (500 LOC, 3/4 of that is comments). And it has testing implementations and trivial "give me a func() to call" implementations.

The more of us that use it (klog already uses it internally, for example :) the more inertia we build.

Writing log wrappers is a nerd-snipe.

While zapr exists, I opted to choose logrusr as logrus just happens to be used by a few more projects in the ecosystem that I'm familiar with (including Kubernetes Release Engineering logging)

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Yes!

Consumers of functions like:

func CreateGithubRepoClient(ctx context.Context, logger *zap.Logger) clients.RepoClient { ... }

will need to use log.Logger as an argument:

func CreateGithubRepoClient(ctx context.Context, logger *log.Logger) clients.RepoClient { ... }

as well as update log calls from:

ghClient := CreateGithubRepoClient(ctx, logger)
logger.Info(...)

to:

ghClient := CreateGithubRepoClient(ctx, logger)
logger.Zap.Info(...)

Other information:

github-actions · 2022-01-23T11:41:42Z

Integration tests success for
[d413eea]
(https://github.com/ossf/scorecard/actions/runs/1735784633)

github-actions · 2022-01-23T12:00:30Z

Integration tests success for
[ab7c620]
(https://github.com/ossf/scorecard/actions/runs/1735816942)

github-actions · 2022-01-23T12:43:53Z

Integration tests success for
[b8fd952]
(https://github.com/ossf/scorecard/actions/runs/1735911390)

codecov-commenter · 2022-01-23T13:01:56Z

Codecov Report

Merging #1516 (f85ee75) into main (b6cba86) will increase coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1516      +/-   ##
==========================================
+ Coverage   51.25%   51.32%   +0.07%     
==========================================
  Files          68       68              
  Lines        6140     6131       -9     
==========================================
  Hits         3147     3147              
+ Misses       2788     2779       -9     
  Partials      205      205

log/log.go

naveensrinivasan · 2022-01-24T02:52:37Z

If we merge this change then the next release would have to be v5.0.0. This is breaking API.

github-actions · 2022-01-24T21:53:30Z

Integration tests success for
[3ab622a]
(https://github.com/ossf/scorecard/actions/runs/1742343093)

github-actions · 2022-01-25T00:39:56Z

Integration tests success for
[af4b787]
(https://github.com/ossf/scorecard/actions/runs/1742927281)

justaugustus · 2022-01-25T00:58:33Z

@naveensrinivasan @azeemshaikh38 -- I think this is ready for review!

github-actions · 2022-01-25T01:03:07Z

Integration tests success for
[f85ee75]
(https://github.com/ossf/scorecard/actions/runs/1743003485)

azeemshaikh38

This is so much neater, thanks @justaugustus!

azeemshaikh38 · 2022-01-25T15:41:46Z

go.mod

@@ -35,6 +37,8 @@ require (
 	mvdan.cc/sh/v3 v3.4.2
 )

+// TODO(go.mod): Is there a reason these deps are kept separately from the


@naveensrinivasan might know why?

It looks like they might be related to tools, which is why they got segmented off?
Anyhow, I kept the TODO in and moved them to the top in bba907d, which should cause go mod tidy to update the larger replace block instead.

azeemshaikh38 · 2022-01-25T15:47:35Z

If we merge this change then the next release would have to be v5.0.0. This is breaking API.

True. For now, I think we can merge this in and discuss in this week's meet about how to handle the release version.

Signed-off-by: Stephen Augustus <[email protected]>

...to prevent automatic updates from getting added to the smaller section. Signed-off-by: Stephen Augustus <[email protected]>

justaugustus · 2022-01-25T16:33:54Z

This is so much neater, thanks @justaugustus!

@azeemshaikh38 -- Happy to help! I just rebased to deconflict w/ main, so once tests pass, this should be good to merge.

github-actions · 2022-01-25T16:36:28Z

Integration tests success for
[bba907d]
(https://github.com/ossf/scorecard/actions/runs/1746537081)

naveensrinivasan

Thanks!

naveensrinivasan · 2022-01-25T17:16:50Z

log/log.go

+// If the level is not recognized, it defaults to `logrus.InfoLevel` to swallow
+// potential configuration errors/typos when specifying log levels.
+// https://pkg.go.dev/github.com/sirupsen/logrus#ParseLevel
+func ParseLevel(lvl string) logrus.Level {


Could we add some unit tests for this?

Could we add some unit tests for this?

Created #1528 for tracking!

justaugustus temporarily deployed to integration-test January 23, 2022 11:37 Inactive

justaugustus temporarily deployed to integration-test January 23, 2022 11:56 Inactive

justaugustus force-pushed the zap-zapped branch from ab7c620 to b8fd952 Compare January 23, 2022 12:38

justaugustus temporarily deployed to integration-test January 23, 2022 12:39 Inactive

naveensrinivasan reviewed Jan 23, 2022

View reviewed changes

log/log.go Outdated Show resolved Hide resolved

justaugustus force-pushed the zap-zapped branch from b8fd952 to 3ab622a Compare January 24, 2022 21:48

justaugustus temporarily deployed to integration-test January 24, 2022 21:49 Inactive

justaugustus force-pushed the zap-zapped branch from 3ab622a to af4b787 Compare January 25, 2022 00:34

justaugustus temporarily deployed to integration-test January 25, 2022 00:34 Inactive

justaugustus force-pushed the zap-zapped branch from af4b787 to f85ee75 Compare January 25, 2022 00:58

justaugustus temporarily deployed to integration-test January 25, 2022 00:58 Inactive

justaugustus changed the title ~~⚠️ [WIP] log: Initial logr/logrusr implementation~~ ⚠️ log: Initial logr/logrusr implementation Jan 25, 2022

azeemshaikh38 approved these changes Jan 25, 2022

View reviewed changes

justaugustus added 3 commits January 25, 2022 11:26

log: Initial logr/logrusr implementation

3677fb2

Signed-off-by: Stephen Augustus <[email protected]>

log: Update references to log.Logger

77b1518

Signed-off-by: Stephen Augustus <[email protected]>

go.mod: Minor reorganization of replaces

bba907d

...to prevent automatic updates from getting added to the smaller section. Signed-off-by: Stephen Augustus <[email protected]>

justaugustus force-pushed the zap-zapped branch from f85ee75 to bba907d Compare January 25, 2022 16:30

justaugustus temporarily deployed to integration-test January 25, 2022 16:30 Inactive

justaugustus mentioned this pull request Jan 25, 2022

[WIP] binary: Replace zap with scorecard.log.Logger ossf/allstar#102

Closed

naveensrinivasan approved these changes Jan 25, 2022

View reviewed changes

naveensrinivasan merged commit 41adfe7 into ossf:main Jan 25, 2022

This was referenced Jan 25, 2022

Update to github.com/ossf/scorecard/v4 (take two) ossf/allstar#101

Merged

testing: Add unit tests for log package #1528

Closed

community: Is there a contributor ladder? #1529

Closed

justaugustus deleted the zap-zapped branch January 26, 2022 07:16

justaugustus mentioned this pull request Jan 29, 2022

Should scorecard be forcing users to use a zap.Logger? #1273

Closed

justaugustus mentioned this pull request Feb 16, 2022

Release Scorecard - Next release (v4.1.0) #1621

Closed

spencerschrock mentioned this pull request Nov 8, 2023

Cleanup verbosity level #717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ log: Initial `logr`/`logrusr` implementation #1516

⚠️ log: Initial `logr`/`logrusr` implementation #1516

justaugustus commented Jan 23, 2022 •

edited

Loading

github-actions bot commented Jan 23, 2022

github-actions bot commented Jan 23, 2022

github-actions bot commented Jan 23, 2022

codecov-commenter commented Jan 23, 2022 •

edited

Loading

naveensrinivasan commented Jan 24, 2022

github-actions bot commented Jan 24, 2022

github-actions bot commented Jan 25, 2022

justaugustus commented Jan 25, 2022

github-actions bot commented Jan 25, 2022

azeemshaikh38 left a comment

azeemshaikh38 Jan 25, 2022

justaugustus Jan 25, 2022

azeemshaikh38 commented Jan 25, 2022

justaugustus commented Jan 25, 2022

github-actions bot commented Jan 25, 2022

naveensrinivasan left a comment

naveensrinivasan Jan 25, 2022

justaugustus Jan 25, 2022

⚠️ log: Initial logr/logrusr implementation #1516

⚠️ log: Initial logr/logrusr implementation #1516

Conversation

justaugustus commented Jan 23, 2022 • edited Loading

github-actions bot commented Jan 23, 2022

github-actions bot commented Jan 23, 2022

github-actions bot commented Jan 23, 2022

codecov-commenter commented Jan 23, 2022 • edited Loading

Codecov Report

naveensrinivasan commented Jan 24, 2022

github-actions bot commented Jan 24, 2022

github-actions bot commented Jan 25, 2022

justaugustus commented Jan 25, 2022

github-actions bot commented Jan 25, 2022

azeemshaikh38 left a comment

Choose a reason for hiding this comment

azeemshaikh38 Jan 25, 2022

Choose a reason for hiding this comment

justaugustus Jan 25, 2022

Choose a reason for hiding this comment

azeemshaikh38 commented Jan 25, 2022

justaugustus commented Jan 25, 2022

github-actions bot commented Jan 25, 2022

naveensrinivasan left a comment

Choose a reason for hiding this comment

naveensrinivasan Jan 25, 2022

Choose a reason for hiding this comment

justaugustus Jan 25, 2022

Choose a reason for hiding this comment

⚠️ log: Initial `logr`/`logrusr` implementation #1516

⚠️ log: Initial `logr`/`logrusr` implementation #1516

justaugustus commented Jan 23, 2022 •

edited

Loading

codecov-commenter commented Jan 23, 2022 •

edited

Loading