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

Use zerolog instead of log #766

Merged
merged 1 commit into from
Sep 29, 2022
Merged

Use zerolog instead of log #766

merged 1 commit into from
Sep 29, 2022

Conversation

julienduchesne
Copy link
Member

@julienduchesne julienduchesne commented Sep 27, 2022

Exposes a --log-level arg on every command
Replaces all log uses with either fmt.Fprint(os.Stderr) or zerolog calls

Benefits:

  • We're currently mixing actual logs (that should have a level/are purely informational) and things that we want to print to stderr (possibly parsed by user). Using zerolog and fmt is more deliberate in meaning
  • Logs can be disabled or turned on using the --log-level option
  • We'll be able to add more verbose logs without potentially annoying people. An example is the "Loading environment" messages that the export command now prints. I shifted that to debug
  • Will allow for trace level logs. Ex: Feature: Add a flag for debug logs #751, logs from the underlying kubectl exec or helm

Exposes a `--log-level` arg on every command
Replaces all `log` uses with either `fmt.Fprint(os.Stderr)` or zerolog calls

Benefits:
- Rather than mixing actual logs (that should have a level) and things that we want to print to stderr, using zerolog and fmt is more deliberate in meaning
- Logs can be disabled or turned on using the `--log-level` option
- We'll be able to add more verbose logs without potentially annoying people. An example is the "Loading environment" messages that the export command now prints. I shifted that to debug
- Will allow for trace level logs. Ex: #751, logs from the underlying `kubectl` exec or helm
@julienduchesne julienduchesne requested review from Duologic, sh0rez and a team September 27, 2022 22:14
@julienduchesne julienduchesne marked this pull request as ready for review September 27, 2022 22:16
@github-actions
Copy link

Benchstat (compared to main):

name                                 old time/op    new time/op    delta
pkg:github.com/grafana/tanka/pkg/jsonnet goos:linux goarch:amd64
GetSnippetHash-2                       19.4ms ± 2%    19.4ms ± 1%    ~     (p=0.631 n=10+10)
pkg:github.com/grafana/tanka/pkg/tanka goos:linux goarch:amd64
ExportEnvironmentsWithReplaceEnvs-2    7.09ms ± 1%    7.12ms ± 4%    ~     (p=0.661 n=9+10)

name                                 old alloc/op   new alloc/op   delta
pkg:github.com/grafana/tanka/pkg/jsonnet goos:linux goarch:amd64
GetSnippetHash-2                       3.85MB ± 0%    3.85MB ± 0%    ~     (p=0.631 n=10+10)
pkg:github.com/grafana/tanka/pkg/tanka goos:linux goarch:amd64
ExportEnvironmentsWithReplaceEnvs-2    2.04MB ± 0%    2.04MB ± 0%    ~     (p=0.075 n=10+10)

name                                 old allocs/op  new allocs/op  delta
pkg:github.com/grafana/tanka/pkg/jsonnet goos:linux goarch:amd64
GetSnippetHash-2                        32.3k ± 0%     32.3k ± 0%    ~     (p=0.838 n=10+10)
pkg:github.com/grafana/tanka/pkg/tanka goos:linux goarch:amd64
ExportEnvironmentsWithReplaceEnvs-2     23.5k ± 0%     23.5k ± 0%  -0.06%  (p=0.000 n=10+10)

Copy link
Contributor

@zzehring zzehring left a comment

Choose a reason for hiding this comment

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

LGTM

@julienduchesne julienduchesne merged commit cc21323 into main Sep 29, 2022
@julienduchesne julienduchesne deleted the julienduchesne/zerolog branch September 29, 2022 12:05
julienduchesne added a commit that referenced this pull request Sep 30, 2022
Follow-up to #766
- Shift linting name logging to debug logging. Instead of sending everything to stdout, we can now have a cleaner "report"
- Missed some `log.Printf`(alias for Debug) in helm code, shifted that to info logs
- Added some caching logs, whether or not it's a cache hit + time elasped to calculate hash
- Changed `time` for `duration_ms` in env loading log. This conflicts with the log time
julienduchesne added a commit that referenced this pull request Sep 30, 2022
Follow-up to #766
- Shift linting name logging to debug logging. Instead of sending everything to stdout, we can now have a cleaner "report"
- Missed some `log.Printf`(alias for Debug) in helm code, shifted that to info logs
- Added some caching logs, whether or not it's a cache hit + time elasped to calculate hash
- Changed `time` for `duration_ms` in env loading log. This conflicts with the log time
julienduchesne added a commit that referenced this pull request Oct 3, 2022
Follow-up to #766
- Shift linting name logging to debug logging. Instead of sending everything to stdout, we can now have a cleaner "report"
- Missed some `log.Printf`(alias for Debug) in helm code, shifted that to info logs
- Added some caching logs, whether or not it's a cache hit + time elasped to calculate hash
- Changed `time` for `duration_ms` in env loading log. This conflicts with the log time
Comment on lines -56 to +74
log.Fatalln(color.RedString("Error:"), err)
fmt.Fprintln(os.Stderr, "Error:", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this change makes it so that if there is any errors, we don't exit (1).

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.

4 participants