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

Introduce log levels #560

Closed
halvards opened this issue Dec 22, 2021 · 4 comments
Closed

Introduce log levels #560

halvards opened this issue Dec 22, 2021 · 4 comments

Comments

@halvards
Copy link
Collaborator

When using ko as a library, it can be helpful to have more control of the log output.

If a downstream tool enables debug or trace level logging, it could then propagate this setting to ko. Likewise, a downstream warning log level could translate to having ko only output warnings (e.g., with #542).

For instance, while developing features for #348 and GoogleContainerTools/skaffold#6041, I often added debug log lines to ko that I removed before committing changes.

One approach would be to follow the approach in ggcr: https://github.com/google/go-containerregistry/blob/v0.7.0/pkg/logs/logs.go#L24-L31

This would also allow downstream tools to plug in their logging libraries by sending ko log output to another destination using log/Logger.SetOutput, like what ko does with ggcr: https://github.com/google/ko/blob/v0.9.3/cmd/ko/main.go#L39-L40

And just to be clear, I'm not suggesting introducing a logging library!

@jonjohnsonjr
Copy link
Collaborator

I have some regrets about the global logging stuff in ggcr. I would be fine with adopting https://github.com/go-logr/logr if we want to expose this.

@halvards
Copy link
Collaborator Author

What regrets do you have with the ggcr approach? I like the simplicity of it, which unsurprisingly aligns with ko 😄.

On https://github.com/go-logr/logr, libraries/SDKs should log to interfaces IMHO, so that aligns with my principles, but that'll be a bigger change. Is the goal to inject logger instances rather than reaching for global variables? Or do you see benefits in the structured logging or numbered levels?

@jonjohnsonjr
Copy link
Collaborator

What regrets do you have with the ggcr approach? I like the simplicity of it, which unsurprisingly aligns with ko 😄.

The fact that's it's package-level globals is kinda gross. I'm not sure if adding a logger option is much better, but I do hate globals :)

but that'll be a bigger change

Agreed... I'm assuming you'd want to expose this for use by skaffold? Otherwise we could keep it internal.

Is the goal to inject logger instances rather than reaching for global variables?

Yup!

halvards added a commit to halvards/ko that referenced this issue Jan 25, 2022
This change adds a dependency on `go-logr/logr` and replaces all uses of
the `log` package from Go's standard library with a `logr`-based
implementation.

To ease the transition, and to keep the diff small, this change
introduces a package `github.com/google/ko/pkg/log` that should be
imported in place of `log` from the standard library.

The new package provides some of the same functions as `log`, e.g.,
`Printf()` and `Fatalf()`. The replacement functions takes an extra
`context.Context` argument. The `Context` is used to look up the
`logr.Logger` to use for logging.

Existing log statements have been updated to use the transition
functions, with `Context` plumbed through where required.

New code can use the new logging syntax, e.g.:

```go
log.L(ctx).Info("something interesting happened", "key", "value")
```

or

```go
log.L(ctx).Error(err, "uh oh", "what", "snafu")
```

This change alters the log output to `logr`'s structured output, with
named fields for the log message and level. As an example, a log line
that previously looked like this:

```
2022/01/25 19:44:18 Building github.com/google/ko/test for linux/amd64
```

Will look like as follows with this change:

```
2022/01/25 19:46:02 ko: "level"=1 "msg"="Building github.com/google/ko/test for linux/amd64"
```

Fixes: ko-build#560
Related: ko-build#542
halvards added a commit to halvards/ko that referenced this issue Jan 25, 2022
This change adds a dependency on `go-logr/logr` and replaces all uses of
the `log` package from Go's standard library with a `logr`-based
implementation.

To ease the transition, and to keep the diff small, this change
introduces a package `github.com/google/ko/pkg/log` that should be
imported in place of `log` from the standard library.

The new package provides some of the same functions as `log`, e.g.,
`Printf()` and `Fatalf()`. The replacement functions takes an extra
`context.Context` argument. The `Context` is used to look up the
`logr.Logger` to use for logging.

Existing log statements have been updated to use the transition
functions, with `Context` plumbed through where required.

New code can use the new logging syntax, e.g.:

```go
log.L(ctx).Info("something interesting happened", "key", "value")
```

or

```go
log.L(ctx).Error(err, "uh oh", "what", "snafu")
```

This change alters the log output to `logr`'s structured output, with
named fields for the log message and level. As an example, a log line
that previously looked like this:

```
2022/01/25 19:44:18 Building github.com/google/ko/test for linux/amd64
```

Will look like as follows with this change:

```
2022/01/25 19:46:02 ko: "level"=1 "msg"="Building github.com/google/ko/test for linux/amd64"
```

Fixes: ko-build#560
Related: ko-build#542
halvards added a commit to halvards/ko that referenced this issue Jan 25, 2022
This change adds a dependency on `go-logr/logr` and replaces all uses of
the `log` package from Go's standard library with a `logr`-based
implementation.

To ease the transition, and to keep the diff small, this change
introduces a package `github.com/google/ko/pkg/log` that should be
imported in place of `log` from the standard library.

The new package provides some of the same functions as `log`, e.g.,
`Printf()` and `Fatalf()`. The replacement functions takes an extra
`context.Context` argument. The `Context` is used to look up the
`logr.Logger` to use for logging.

Existing log statements have been updated to use the transition
functions, with `Context` plumbed through where required.

New code can use the new logging syntax, e.g.:

```go
log.L(ctx).Info("something interesting happened", "key", "value")
```

or

```go
log.L(ctx).Error(err, "uh oh", "what", "snafu")
```

This change alters the log output to `logr`'s structured output, with
named fields for the log message and level. As an example, a log line
that previously looked like this:

```
2022/01/25 19:44:18 Building github.com/google/ko/test for linux/amd64
```

Will look like as follows with this change:

```
2022/01/25 19:46:02 ko: "level"=1 "msg"="Building github.com/google/ko/test for linux/amd64"
```

Fixes: ko-build#560
Related: ko-build#542
halvards added a commit to halvards/ko that referenced this issue Jan 25, 2022
This change adds a dependency on `go-logr/logr` and replaces all uses of
the `log` package from Go's standard library with a `logr`-based
implementation.

To ease the transition, and to keep the diff small, this change
introduces a package `github.com/google/ko/pkg/log` that should be
imported in place of `log` from the standard library.

The new package provides some of the same functions as `log`, e.g.,
`Printf()` and `Fatalf()`. The replacement functions takes an extra
`context.Context` argument. The `Context` is used to look up the
`logr.Logger` to use for logging.

Existing log statements have been updated to use the transition
functions, with `Context` plumbed through where required.

New code can use the new logging syntax, e.g.:

```go
log.L(ctx).Info("something interesting happened", "key", "value")
```

or

```go
log.L(ctx).Error(err, "uh oh", "what", "snafu")
```

This change alters the log output to `logr`'s structured output, with
named fields for the log message and level. As an example, a log line
that previously looked like this:

```
2022/01/25 19:44:18 Building github.com/google/ko/test for linux/amd64
```

Will look like as follows with this change:

```
2022/01/25 19:46:02 ko: "level"=1 "msg"="Building github.com/google/ko/test for linux/amd64"
```

Fixes: ko-build#560
Related: ko-build#542
@github-actions
Copy link

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants