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 log/slog instead of zerolog #758

Merged
merged 1 commit into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 22 additions & 13 deletions cmd/pint/ci.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package main
import (
"context"
"fmt"
"log/slog"
"os"
"regexp"
"strconv"
Expand All @@ -15,7 +16,6 @@ import (
"github.com/cloudflare/pint/internal/git"
"github.com/cloudflare/pint/internal/reporter"

"github.com/rs/zerolog/log"
"github.com/urfave/cli/v2"
)

Expand Down Expand Up @@ -82,9 +82,9 @@ func actionCI(c *cli.Context) error {
if err != nil {
return fmt.Errorf("failed to get the name of current branch")
}
log.Debug().Str("current", currentBranch).Str("base", baseBranch).Msg("Got branch information")
slog.Debug("Got branch information", slog.String("base", baseBranch), slog.String("current", currentBranch))
if currentBranch == strings.Split(baseBranch, "/")[len(strings.Split(baseBranch, "/"))-1] {
log.Info().Str("branch", currentBranch).Msg("Running from base branch, skipping checks")
slog.Info("Running from base branch, skipping checks", slog.String("branch", currentBranch))
return nil
}

Expand Down Expand Up @@ -176,15 +176,15 @@ func actionCI(c *cli.Context) error {
}

problemsFound := false
bySeverity := map[string]interface{}{} // interface{} is needed for log.Fields()
for s, c := range summary.CountBySeverity() {
bySeverity := summary.CountBySeverity()
for s := range bySeverity {
if s >= minSeverity {
problemsFound = true
break
}
bySeverity[s.String()] = c
}
if len(bySeverity) > 0 {
log.Info().Fields(bySeverity).Msg("Problems found")
slog.Info("Problems found", logSeverityCounters(bySeverity)...)
}

if err := submitReports(reps, summary); err != nil {
Expand All @@ -198,6 +198,15 @@ func actionCI(c *cli.Context) error {
return nil
}

func logSeverityCounters(src map[checks.Severity]int) (attrs []any) {
for _, s := range []checks.Severity{checks.Fatal, checks.Bug, checks.Warning, checks.Information} {
if c, ok := src[s]; ok {
attrs = append(attrs, slog.Attr{Key: s.String(), Value: slog.IntValue(c)})
}
}
return attrs
}

func detectCI(cfg *config.CI) *config.CI {
var isNil, isDirty bool

Expand All @@ -209,7 +218,7 @@ func detectCI(cfg *config.CI) *config.CI {
if bb := os.Getenv("GITHUB_BASE_REF"); bb != "" {
isDirty = true
cfg.BaseBranch = bb
log.Debug().Str("branch", bb).Msg("got base branch from GITHUB_BASE_REF env variable")
slog.Debug("got base branch from GITHUB_BASE_REF env variable", slog.String("branch", bb))
}

if isNil && !isDirty {
Expand Down Expand Up @@ -243,7 +252,7 @@ func detectGithubActions(gh *config.GitHub) *config.GitHub {
os.Getenv("GITHUB_REF") != "" {
parts := strings.Split(os.Getenv("GITHUB_REF"), "/")
if len(parts) >= 4 {
log.Info().Str("pr", parts[2]).Msg("Setting GITHUB_PULL_REQUEST_NUMBER from GITHUB_REF env variable")
slog.Info("Setting GITHUB_PULL_REQUEST_NUMBER from GITHUB_REF env variable", slog.String("pr", parts[2]))
os.Setenv("GITHUB_PULL_REQUEST_NUMBER", parts[2])
}
}
Expand All @@ -259,12 +268,12 @@ func detectGithubActions(gh *config.GitHub) *config.GitHub {
parts := strings.SplitN(repo, "/", 2)
if len(parts) == 2 {
if gh.Owner == "" {
log.Info().Str("owner", parts[0]).Msg("Setting repository owner from GITHUB_REPOSITORY env variable")
slog.Info("Setting repository owner from GITHUB_REPOSITORY env variable", slog.String("owner", parts[0]))
gh.Owner = parts[0]
isDirty = true
}
if gh.Repo == "" {
log.Info().Str("repo", parts[1]).Msg("Setting repository name from GITHUB_REPOSITORY env variable")
slog.Info("Setting repository name from GITHUB_REPOSITORY env variable", slog.String("repo", parts[1]))
gh.Repo = parts[1]
isDirty = true
}
Expand All @@ -273,11 +282,11 @@ func detectGithubActions(gh *config.GitHub) *config.GitHub {

if api := os.Getenv("GITHUB_API_URL"); api != "" {
if gh.BaseURI == "" {
log.Info().Str("baseuri", api).Msg("Setting repository base URI from GITHUB_API_URL env variable")
slog.Info("Setting repository base URI from GITHUB_API_URL env variable", slog.String("baseuri", api))
gh.BaseURI = api
}
if gh.UploadURI == "" {
log.Info().Str("uploaduri", api).Msg("Setting repository upload URI from GITHUB_API_URL env variable")
slog.Info("Setting repository upload URI from GITHUB_API_URL env variable", slog.String("uploaduri", api))
gh.UploadURI = api
}
}
Expand Down
11 changes: 5 additions & 6 deletions cmd/pint/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package main
import (
"context"
"fmt"
"log/slog"
"os"
"regexp"

Expand All @@ -11,7 +12,6 @@ import (
"github.com/cloudflare/pint/internal/discovery"
"github.com/cloudflare/pint/internal/reporter"

"github.com/rs/zerolog/log"
"github.com/urfave/cli/v2"
)

Expand Down Expand Up @@ -87,25 +87,24 @@ func actionLint(c *cli.Context) error {
return err
}

bySeverity := map[string]interface{}{} // interface{} is needed for log.Fields()
bySeverity := summary.CountBySeverity()
var problems, hiddenProblems, failProblems int
for s, c := range summary.CountBySeverity() {
for s, c := range bySeverity {
if s >= failOn {
failProblems++
}
if s < minSeverity {
hiddenProblems++
}
bySeverity[s.String()] = c
if s >= checks.Bug {
problems += c
}
}
if len(bySeverity) > 0 {
log.Info().Fields(bySeverity).Msg("Problems found")
slog.Info("Problems found", logSeverityCounters(bySeverity)...)
}
if hiddenProblems > 0 {
log.Info().Msgf("%d problem(s) not visible because of --%s=%s flag", hiddenProblems, minSeverityFlag, c.String(minSeverityFlag))
slog.Info(fmt.Sprintf("%d problem(s) not visible because of --%s=%s flag", hiddenProblems, minSeverityFlag, c.String(minSeverityFlag)))
}

if failProblems > 0 {
Expand Down
34 changes: 9 additions & 25 deletions cmd/pint/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,37 +4,21 @@ import (
"fmt"
"os"

"github.com/rs/zerolog"
"github.com/rs/zerolog/log"
"github.com/cloudflare/pint/internal/log"
)

func msgFormatter(msg interface{}) string {
return fmt.Sprintf("msg=%q", msg)
}

func lvlFormatter(level interface{}) string {
if level == nil {
return ""
}
return fmt.Sprintf("level=%s", level)
}

func initLogger(level string, noColor bool) error {
log.Logger = log.Logger.Output(zerolog.ConsoleWriter{
Out: os.Stderr,
NoColor: noColor,
FormatLevel: lvlFormatter,
FormatMessage: msgFormatter,
FormatTimestamp: func(interface{}) string {
return ""
},
})

l, err := zerolog.ParseLevel(level)
l, err := log.ParseLevel(level)
if err != nil {
return fmt.Errorf("'%s' is not a valid log level", level)
}
zerolog.SetGlobalLevel(l)

nc := os.Getenv("NO_COLOR")
if nc != "" && nc != "0" {
noColor = true
}

log.Setup(l, noColor)

return nil
}
9 changes: 4 additions & 5 deletions cmd/pint/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@ package main

import (
"fmt"
"log/slog"
"os"

"github.com/rs/zerolog"
"github.com/rs/zerolog/log"
"github.com/urfave/cli/v2"
"go.uber.org/automaxprocs/maxprocs"

Expand Down Expand Up @@ -45,7 +44,7 @@ func newApp() *cli.App {
&cli.StringFlag{
Name: logLevelFlag,
Aliases: []string{"l"},
Value: zerolog.InfoLevel.String(),
Value: slog.LevelInfo.String(),
Usage: "Log level",
},
&cli.BoolFlag{
Expand Down Expand Up @@ -98,7 +97,7 @@ func actionSetup(c *cli.Context) (meta actionMeta, err error) {
undo, err := maxprocs.Set()
defer undo()
if err != nil {
log.Error().Err(err).Msg("failed to set GOMAXPROCS")
slog.Error("failed to set GOMAXPROCS", slog.Any("err", err))
}

meta.workers = c.Int(workersFlag)
Expand All @@ -122,7 +121,7 @@ func main() {
app := newApp()
err := app.Run(os.Args)
if err != nil {
log.Fatal().Err(err).Msg("Execution completed with error(s)")
slog.Error("Execution completed with error(s)", slog.Any("err", err))
os.Exit(1)
}
}
7 changes: 3 additions & 4 deletions cmd/pint/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"encoding/pem"
"errors"
"fmt"
"log/slog"
"math/big"
"net"
"net/http"
Expand All @@ -20,16 +21,14 @@ import (
"time"

"github.com/rogpeppe/go-internal/testscript"
"github.com/rs/zerolog"
"github.com/rs/zerolog/log"
)

// mock command that fails tests if error is returned
func mockMainShouldSucceed() int {
app := newApp()
err := app.Run(os.Args)
if err != nil {
log.WithLevel(zerolog.FatalLevel).Err(err).Msg("Fatal error")
slog.Error("Fatal error", slog.Any("err", err))
return 1
}
return 0
Expand All @@ -40,7 +39,7 @@ func mockMainShouldFail() int {
app := newApp()
err := app.Run(os.Args)
if err != nil {
log.WithLevel(zerolog.FatalLevel).Err(err).Msg("Fatal error")
slog.Error("Fatal error", slog.Any("err", err))
return 0
}
fmt.Fprintf(os.Stderr, "expected an error but none was returned\n")
Expand Down
30 changes: 15 additions & 15 deletions cmd/pint/scan.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ package main
import (
"context"
"errors"
"log/slog"
"regexp"
"strconv"
"sync"
"time"

"github.com/prometheus/prometheus/model/rulefmt"
"github.com/rs/zerolog/log"
"go.uber.org/atomic"

"github.com/cloudflare/pint/internal/checks"
Expand Down Expand Up @@ -93,19 +93,19 @@ func checkRules(ctx context.Context, workers int, cfg config.Config, entries []d
case entry.PathError == nil && entry.Rule.Error.Err == nil:
if entry.Rule.RecordingRule != nil {
rulesParsedTotal.WithLabelValues(config.RecordingRuleType).Inc()
log.Debug().
Str("path", entry.SourcePath).
Str("record", entry.Rule.RecordingRule.Record.Value.Value).
Str("lines", output.FormatLineRangeString(entry.Rule.Lines())).
Msg("Found recording rule")
slog.Debug("Found recording rule",
slog.String("path", entry.SourcePath),
slog.String("record", entry.Rule.RecordingRule.Record.Value.Value),
slog.String("lines", output.FormatLineRangeString(entry.Rule.Lines())),
)
}
if entry.Rule.AlertingRule != nil {
rulesParsedTotal.WithLabelValues(config.AlertingRuleType).Inc()
log.Debug().
Str("path", entry.SourcePath).
Str("alert", entry.Rule.AlertingRule.Alert.Value.Value).
Str("lines", output.FormatLineRangeString(entry.Rule.Lines())).
Msg("Found alerting rule")
slog.Debug("Found alerting rule",
slog.String("path", entry.SourcePath),
slog.String("alert", entry.Rule.AlertingRule.Alert.Value.Value),
slog.String("lines", output.FormatLineRangeString(entry.Rule.Lines())),
)
}

checkList := cfg.GetChecksForRule(ctx, entry.SourcePath, entry.Rule, entry.DisabledChecks)
Expand All @@ -121,10 +121,10 @@ func checkRules(ctx context.Context, workers int, cfg config.Config, entries []d
}
default:
if entry.Rule.Error.Err != nil {
log.Debug().
Str("path", entry.SourcePath).
Str("lines", output.FormatLineRangeString(entry.Rule.Lines())).
Msg("Found invalid rule")
slog.Debug("Found invalid rule",
slog.String("path", entry.SourcePath),
slog.String("lines", output.FormatLineRangeString(entry.Rule.Lines())),
)
rulesParsedTotal.WithLabelValues(config.InvalidRuleType).Inc()
}
jobs <- scanJob{entry: entry, allEntries: entries, check: nil}
Expand Down
6 changes: 3 additions & 3 deletions cmd/pint/tests/0001_match_path.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ pint.error --no-color lint rules
cmp stderr stderr.txt

-- stderr.txt --
level=info msg="Loading configuration file" path=.pint.hcl
level=INFO msg="Loading configuration file" path=.pint.hcl
rules/0002.yml:2 Bug: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
2 | expr: sum(foo) without(job)

level=info msg="Problems found" Bug=1
level=fatal msg="Fatal error" error="found 1 problem(s) with severity Bug or higher"
level=INFO msg="Problems found" Bug=1
level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"
-- rules/0001.yml --
- record: "colo:test1"
expr: sum(foo) without(job)
Expand Down
2 changes: 1 addition & 1 deletion cmd/pint/tests/0002_nothing_to_lint.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ pint.error --no-color lint rules
cmp stderr stderr.txt

-- stderr.txt --
level=fatal msg="Fatal error" error="no matching files"
level=ERROR msg="Fatal error" err="no matching files"
7 changes: 4 additions & 3 deletions cmd/pint/tests/0003_lint_workdir.txt
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
env NO_COLOR=1
pint.error --no-color lint --min-severity=info rules
! stdout .
cmp stderr stderr.txt

-- stderr.txt --
level=info msg="Loading configuration file" path=.pint.hcl
level=INFO msg="Loading configuration file" path=.pint.hcl
rules/0001.yml:2 Warning: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
2 | expr: sum(rate(fl_cf_html_bytes_in[10m])) WITHOUT (colo_id, instance, node_type, region, node_status, job, colo_name)

Expand Down Expand Up @@ -60,8 +61,8 @@ rules/0003.yaml:58-61 Information: using the value of rate(errors[5m]) inside th
..
61 | summary: 'error rate: {{ $value }}'

level=info msg="Problems found" Bug=2 Fatal=1 Information=1 Warning=10
level=fatal msg="Fatal error" error="found 2 problem(s) with severity Bug or higher"
level=INFO msg="Problems found" Fatal=1 Bug=2 Warning=10 Information=1
level=ERROR msg="Fatal error" err="found 2 problem(s) with severity Bug or higher"
-- rules/0001.yml --
- record: colo_job:fl_cf_html_bytes_in:rate10m
expr: sum(rate(fl_cf_html_bytes_in[10m])) WITHOUT (colo_id, instance, node_type, region, node_status, job, colo_name)
Expand Down
Loading
Loading