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

[naveensrinivasan]

External packages like Allstar would like to use as a scorecard as a Library.
https://github.com/ossf/allstar/blob/913bb71913d373d0daaf2a59957c3610d2522e50/pkg/policies/binary/binary.go#L120

scorecard/clients/githubrepo/client.go

Line 183 in 1050b1c

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

This API call is forcing the consumers to use zap.Logger. This should not be forced to use a specific logger. It could be interface for logging, not a specific implementation.

[laurentsimon]

I agree it should not force users to use zap.Logger.

[naveensrinivasan]

@azeemshaikh38 FYI...

[jeffmendoza]

Ideally CreateGitHubRepoClient() can be refactored to take a DetailLogger instead of a zap.Logger.

[github-actions]

Stale issue message

[justaugustus]

I might be able to pick this up after ossf/allstar#98.
(Just commenting to shake up the stale bot...)

[azeemshaikh38]

Thanks @justaugustus!

[justaugustus]

Working on this in #1502.

[justaugustus]

The first bit is merged in #1502.
Before I continue, I want check in on something...

What's the motivation behind

scorecard/checker/detail_logger_impl.go

Lines 42 to 50 in 0d76dea

	// UPGRADEv3: to rename.
	func (l *logger) Info3(msg *LogMessage) {
	cd := CheckDetail{
	Type: DetailInfo,
	Msg: *msg,
	}
	cd.Msg.Version = 3
	l.messages2 = append(l.messages2, cd)
	}

?

More specifically, the function argument, (msg *LogMessage):

func (l *logger) Info3(msg *LogMessage) { ... }

*LogMessage makes it hard to do logger.Info3() without first doing something like:

log := &LoggerMessage{}
log.Text = "the thing I want to log"

logger.Info3(log)

I just want to make sure I'm not missing something about the intended ergonomics, before trying to implement.

Perhaps something variadic could work nicely here?

// Adds a struct of fields to the log entry. All it does is call `WithField` for
// each `Field`.
func (logger *Logger) WithFields(fields Fields) *Entry {
	entry := logger.newEntry()
	defer logger.releaseEntry(entry)
	return entry.WithFields(fields)
}

ref: https://github.com/sirupsen/logrus/blob/85981c045988a3ebe607e2f5b8a6499d86e0ddac/logger.go#L121-L127

e.g.,

logger := log.Logger().WithFields()

where the fields are roughly the fields of the LogMessage struct.

---

Another question...
Is the intent to use

scorecard/checker/detail_logger.go

Lines 17 to 28 in 0d76dea

	// DetailLogger logs a CheckDetail struct.
	type DetailLogger interface {
	Info(desc string, args ...interface{})
	Warn(desc string, args ...interface{})
	Debug(desc string, args ...interface{})
	// Functions to use for moving to SARIF format.
	// UPGRADEv3: to rename.
	Info3(msg *LogMessage)
	Warn3(msg *LogMessage)
	Debug3(msg *LogMessage)
	}

for all program logs or just the checker?

[laurentsimon]

The first bit is merged in #1502. Before I continue, I want check in on something...

What's the motivation behind

scorecard/checker/detail_logger_impl.go

Lines 42 to 50 in 0d76dea

	// UPGRADEv3: to rename.
	func (l *logger) Info3(msg *LogMessage) {
	cd := CheckDetail{
	Type: DetailInfo,
	Msg: *msg,
	}
	cd.Msg.Version = 3
	l.messages2 = append(l.messages2, cd)
	}

?
More specifically, the function argument, (msg *LogMessage):

func (l *logger) Info3(msg *LogMessage) { ... }

*LogMessage makes it hard to do logger.Info3() without first doing something like:

log := &LoggerMessage{}
log.Text = "the thing I want to log"

logger.Info3(log)

I just want to make sure I'm not missing something about the intended ergonomics, before trying to implement.

the motivation for the structure is that we wanted to be able to add fields to the function without changing the function signature - because it requires going thru all calls and updating them :/

Perhaps something variadic could work nicely here?

// Adds a struct of fields to the log entry. All it does is call `WithField` for
// each `Field`.
func (logger *Logger) WithFields(fields Fields) *Entry {
	entry := logger.newEntry()
	defer logger.releaseEntry(entry)
	return entry.WithFields(fields)
}

ref: https://github.com/sirupsen/logrus/blob/85981c045988a3ebe607e2f5b8a6499d86e0ddac/logger.go#L121-L127

e.g.,

logger := log.Logger().WithFields()

I think variadic function would work. Is there a way to fix the list of acceptable fields in order to detect typo mistakes at compilation time?

where the fields are roughly the fields of the LogMessage struct.

Another question... Is the intent to use

scorecard/checker/detail_logger.go

Lines 17 to 28 in 0d76dea

	// DetailLogger logs a CheckDetail struct.
	type DetailLogger interface {
	Info(desc string, args ...interface{})
	Warn(desc string, args ...interface{})
	Debug(desc string, args ...interface{})
	// Functions to use for moving to SARIF format.
	// UPGRADEv3: to rename.
	Info3(msg *LogMessage)
	Warn3(msg *LogMessage)
	Debug3(msg *LogMessage)
	}

for all program logs or just the checker?

We only use if for the checker today, because those "logs" are a way to convey the results of each check. I think it's fair to say this is not going to change.

[justaugustus]

@laurentsimon --

I think variadic function would work.

Okay thanks, that makes this a bit more flexible.

Is there a way to fix the list of acceptable fields in order to detect typo mistakes at compilation time?

I think ideally, we don't expose the opportunity for a mistake.
For each client, there will be an idea of what kind of data you want out of the logs, so we can force the client to do the right things.

WARNING, pseudocode ahead:

func AllOfTheThings() {
    t1 := NewThing1Client()
    t1.Do()

    t2 := NewThing2Client()
    t2.Do()

    t3 := NewThing3Client()
    t3.Do()
}

func NewThing1Client() *Thing1Client {
    logger := log.NewLogger()
    logger.WithFields(/* thing1-specific fields */)

    client := &Thing1Client{
        Log: logger,
    }

    return &client
}

type Thing1Client struct {
    Log log.Logger
    /* other thing1 stuff */
}

func (t1 *Thing1Client) Do() {
    t1.Log.Info("look at all of the thing1 stuff we're doing!")
}

Maybe you expose the log level for configuration, but let give the client the responsibility of setting the context/fields that are unique to it.

We only use if for the checker today, because those "logs" are a way to convey the results of each check. I think it's fair to say this is not going to change.

SGTM!

[thockin]

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.

[laurentsimon]

SGTM. @naveensrinivasan @justaugustus @azeemshaikh38 thoughts?

[justaugustus]

SGTM. @naveensrinivasan @justaugustus @azeemshaikh38 thoughts?

Yep, I'm happy to roll with the https://github.com/go-logr/logr API!
Will wait for another +1 from a maintainer before proceeding.

[thockin]

And, you know, drop me a line if it doesn't satisfy for some reason.

…

On Fri, Jan 21, 2022 at 12:20 PM justaugustus ***@***.***> wrote: SGTM. @naveensrinivasan <https://github.com/naveensrinivasan> @justaugustus <https://github.com/justaugustus> @azeemshaikh38 <https://github.com/azeemshaikh38> thoughts? Yep, I'm happy to roll with the https://github.com/go-logr/logr API! Will wait for another +1 from a maintainer before proceeding. — Reply to this email directly, view it on GitHub <#1273 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVHZNA3JHHYNUTZ6KD3UXG5ZHANCNFSM5ICTWESQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[justaugustus]

And, you know, drop me a line if it doesn't satisfy for some reason.

Hehe, always happy to bug you, Tim! :)

[azeemshaikh38]

SGTM. @naveensrinivasan @justaugustus @azeemshaikh38 thoughts?

I love it! I have not looked into the implementation/code, but from the description it seems to fit our usecase pretty well. It'll definitely help if we can clean up our logging interfaces and piggy back on an existing and well trusted interface instead.

[justaugustus]

Calling this closed w/ #1516.