Skip to content

Commit

Permalink
chore!: adopt log/slog, remove go-kit/log
Browse files Browse the repository at this point in the history
For: prometheus#14355

Requires a new release of prometheus/common to be cut, as this depends
on the following PRs:

prometheus/common#694
prometheus/common#697

This commit updates Prometheus to adopt stdlib's log/slog package in
favor of go-kit/log. As part of converting to use slog, several other
related changes are required to get prometheus working, including:
- removed unused logging util func `RateLimit()`
- forward ported the util/logging/Deduper logging by implementing a small custom slog.Handler that does the deduping before chaining log calls to the underlying real slog.Logger
- move some of the json file logging functionality to use prom/common package functionality
- refactored some of the new json file logging for scraping
- changes to promql.QueryLogger interface to swap out logging methods for relevant slog sugar wrappers
- updated lots of tests that used/replicated custom logging functionality, attempting to keep the logical goal of the tests consistent after the transition
- added a healthy amount of `if logger == nil { $makeLogger }` type conditional checks amongst various functions where none were provided -- old code that used the go-kit/log.Logger interface had several places where there were nil references when trying to use functions like `With()` to add keyvals on the new *slog.Logger type

Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
  • Loading branch information
tjhop committed Sep 27, 2024
1 parent 105ab2e commit b49314d
Show file tree
Hide file tree
Showing 161 changed files with 1,560 additions and 1,681 deletions.
1 change: 1 addition & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ linters:
- usestdlibvars
- whitespace
- loggercheck
- sloglint

issues:
max-issues-per-linter: 0
Expand Down
257 changes: 147 additions & 110 deletions cmd/prometheus/main.go

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions cmd/prometheus/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ import (
"time"

"github.com/alecthomas/kingpin/v2"
"github.com/go-kit/log"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/common/model"
"github.com/prometheus/common/promslog"
"github.com/stretchr/testify/require"

"github.com/prometheus/prometheus/config"
Expand Down Expand Up @@ -295,7 +295,7 @@ func TestTimeMetrics(t *testing.T) {
tmpDir := t.TempDir()

reg := prometheus.NewRegistry()
db, err := openDBWithMetrics(tmpDir, log.NewNopLogger(), reg, nil, nil)
db, err := openDBWithMetrics(tmpDir, promslog.NewNopLogger(), reg, nil, nil)
require.NoError(t, err)
defer func() {
require.NoError(t, db.Close())
Expand Down
40 changes: 40 additions & 0 deletions cmd/prometheus/query_log_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ func (p *queryLogTest) queryString() string {
// test parameters.
func (p *queryLogTest) validateLastQuery(t *testing.T, ql []queryLogLine) {
q := ql[len(ql)-1]
// TODO: does this also need to be edited to account to msg key prefix?
require.Equal(t, p.queryString(), q.Params.Query)

switch p.origin {
Expand Down Expand Up @@ -370,6 +371,44 @@ func (p *queryLogTest) run(t *testing.T) {
}
}

/*
{
"time": "2024-09-10T11:24:59.268073075-04:00",
"level": "INFO",
"source": {
"function": "github.com/prometheus/prometheus/util/logging.(*JSONFileLogger).Info",
"file": "/home/tjhop/go/src/github.com/prometheus/prometheus/util/logging/file.go",
"line": 64
},
"msg": "promql query logged",
"params": {
"end": "2024-09-10T15:24:59.262Z",
"query": "up{job=\"node_exporter\"} != 1",
"start": "2024-09-10T15:24:59.262Z",
"step": 0
},
"stats": {
"timings": {
"evalTotalTime": 0.000054197,
"resultSortTime": 0,
"queryPreparationTime": 0.000012798,
"innerEvalTime": 0.000032033,
"execQueueTime": 0.000009549,
"execTotalTime": 0.000065458
},
"samples": {
"totalQueryableSamples": 1,
"peakSamples": 5
}
},
"spanID": "0000000000000000",
"ruleGroup": {
"file": "test-rule.yml",
"name": "issue-3512"
}
}
*/

type queryLogLine struct {
Params struct {
Query string `json:"query"`
Expand All @@ -393,6 +432,7 @@ func readQueryLog(t *testing.T, path string) []queryLogLine {
file, err := os.Open(path)
require.NoError(t, err)
defer file.Close()

scanner := bufio.NewScanner(file)
for scanner.Scan() {
var q queryLogLine
Expand Down
4 changes: 2 additions & 2 deletions cmd/promtool/backfill.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import (
"math"
"time"

"github.com/go-kit/log"
"github.com/oklog/ulid"

"github.com/prometheus/common/promslog"
"github.com/prometheus/prometheus/model/labels"
"github.com/prometheus/prometheus/model/textparse"
"github.com/prometheus/prometheus/tsdb"
Expand Down Expand Up @@ -120,7 +120,7 @@ func createBlocks(input []byte, mint, maxt, maxBlockDuration int64, maxSamplesIn
// also need to append samples throughout the whole block range. To allow that, we
// pretend that the block is twice as large here, but only really add sample in the
// original interval later.
w, err := tsdb.NewBlockWriter(log.NewNopLogger(), outputDir, 2*blockDuration)
w, err := tsdb.NewBlockWriter(promslog.NewNopLogger(), outputDir, 2*blockDuration)
if err != nil {
return fmt.Errorf("block writer: %w", err)
}
Expand Down
6 changes: 3 additions & 3 deletions cmd/promtool/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ import (
"time"

"github.com/alecthomas/kingpin/v2"
"github.com/go-kit/log"
"github.com/google/pprof/profile"
"github.com/prometheus/client_golang/api"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/testutil/promlint"
config_util "github.com/prometheus/common/config"
"github.com/prometheus/common/model"
"github.com/prometheus/common/promslog"
"github.com/prometheus/common/version"
"github.com/prometheus/exporter-toolkit/web"
"gopkg.in/yaml.v2"
Expand Down Expand Up @@ -575,7 +575,7 @@ func checkFileExists(fn string) error {
func checkConfig(agentMode bool, filename string, checkSyntaxOnly bool) ([]string, error) {
fmt.Println("Checking", filename)

cfg, err := config.LoadFile(filename, agentMode, false, log.NewNopLogger())
cfg, err := config.LoadFile(filename, agentMode, false, promslog.NewNopLogger())
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1182,7 +1182,7 @@ func importRules(url *url.URL, roundTripper http.RoundTripper, start, end, outpu
return fmt.Errorf("new api client error: %w", err)
}

ruleImporter := newRuleImporter(log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr)), cfg, api)
ruleImporter := newRuleImporter(promslog.New(&promslog.Config{}), cfg, api)
errs := ruleImporter.loadGroups(ctx, files)
for _, err := range errs {
if err != nil {
Expand Down
18 changes: 9 additions & 9 deletions cmd/promtool/rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ package main
import (
"context"
"fmt"
"log/slog"
"time"

"github.com/go-kit/log"
"github.com/go-kit/log/level"
v1 "github.com/prometheus/client_golang/api/prometheus/v1"
"github.com/prometheus/common/model"
"github.com/prometheus/common/promslog"

"github.com/prometheus/prometheus/model/labels"
"github.com/prometheus/prometheus/model/timestamp"
Expand All @@ -38,7 +38,7 @@ type queryRangeAPI interface {
}

type ruleImporter struct {
logger log.Logger
logger *slog.Logger
config ruleImporterConfig

apiClient queryRangeAPI
Expand All @@ -57,8 +57,8 @@ type ruleImporterConfig struct {

// newRuleImporter creates a new rule importer that can be used to parse and evaluate recording rule files and create new series
// written to disk in blocks.
func newRuleImporter(logger log.Logger, config ruleImporterConfig, apiClient queryRangeAPI) *ruleImporter {
level.Info(logger).Log("backfiller", "new rule importer", "start", config.start.Format(time.RFC822), "end", config.end.Format(time.RFC822))
func newRuleImporter(logger *slog.Logger, config ruleImporterConfig, apiClient queryRangeAPI) *ruleImporter {
logger.Info("new rule importer", "component", "backfiller", "start", config.start.Format(time.RFC822), "end", config.end.Format(time.RFC822))
return &ruleImporter{
logger: logger,
config: config,
Expand All @@ -80,10 +80,10 @@ func (importer *ruleImporter) loadGroups(_ context.Context, filenames []string)
// importAll evaluates all the recording rules and creates new time series and writes them to disk in blocks.
func (importer *ruleImporter) importAll(ctx context.Context) (errs []error) {
for name, group := range importer.groups {
level.Info(importer.logger).Log("backfiller", "processing group", "name", name)
importer.logger.Info("processing group", "component", "backfiller", "name", name)

for i, r := range group.Rules() {
level.Info(importer.logger).Log("backfiller", "processing rule", "id", i, "name", r.Name())
importer.logger.Info("processing rule", "component", "backfiller", "id", i, "name", r.Name())
if err := importer.importRule(ctx, r.Query().String(), r.Name(), r.Labels(), importer.config.start, importer.config.end, int64(importer.config.maxBlockDuration/time.Millisecond), group); err != nil {
errs = append(errs, err)
}
Expand Down Expand Up @@ -124,7 +124,7 @@ func (importer *ruleImporter) importRule(ctx context.Context, ruleExpr, ruleName
return fmt.Errorf("query range: %w", err)
}
if warnings != nil {
level.Warn(importer.logger).Log("msg", "Range query returned warnings.", "warnings", warnings)
importer.logger.Warn("Range query returned warnings.", "warnings", warnings)
}

// To prevent races with compaction, a block writer only allows appending samples
Expand All @@ -133,7 +133,7 @@ func (importer *ruleImporter) importRule(ctx context.Context, ruleExpr, ruleName
// also need to append samples throughout the whole block range. To allow that, we
// pretend that the block is twice as large here, but only really add sample in the
// original interval later.
w, err := tsdb.NewBlockWriter(log.NewNopLogger(), importer.config.outputDir, 2*blockDuration)
w, err := tsdb.NewBlockWriter(promslog.NewNopLogger(), importer.config.outputDir, 2*blockDuration)
if err != nil {
return fmt.Errorf("new block writer: %w", err)
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/promtool/rules_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import (
"testing"
"time"

"github.com/go-kit/log"
v1 "github.com/prometheus/client_golang/api/prometheus/v1"
"github.com/prometheus/common/model"
"github.com/prometheus/common/promslog"
"github.com/stretchr/testify/require"

"github.com/prometheus/prometheus/model/labels"
Expand Down Expand Up @@ -161,7 +161,7 @@ func TestBackfillRuleIntegration(t *testing.T) {
}

func newTestRuleImporter(_ context.Context, start time.Time, tmpDir string, testSamples model.Matrix, maxBlockDuration time.Duration) (*ruleImporter, error) {
logger := log.NewNopLogger()
logger := promslog.NewNopLogger()
cfg := ruleImporterConfig{
outputDir: tmpDir,
start: start.Add(-10 * time.Hour),
Expand Down
4 changes: 2 additions & 2 deletions cmd/promtool/sd.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ import (
"os"
"time"

"github.com/go-kit/log"
"github.com/google/go-cmp/cmp"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/common/promslog"

"github.com/prometheus/prometheus/config"
"github.com/prometheus/prometheus/discovery"
Expand All @@ -39,7 +39,7 @@ type sdCheckResult struct {

// CheckSD performs service discovery for the given job name and reports the results.
func CheckSD(sdConfigFiles, sdJobName string, sdTimeout time.Duration, registerer prometheus.Registerer) int {
logger := log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr))
logger := promslog.New(&promslog.Config{})

cfg, err := config.LoadFile(sdConfigFiles, false, false, logger)
if err != nil {
Expand Down
11 changes: 5 additions & 6 deletions cmd/promtool/tsdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"errors"
"fmt"
"io"
"log/slog"
"os"
"path/filepath"
"runtime"
Expand All @@ -32,9 +33,9 @@ import (
"time"

"github.com/alecthomas/units"
"github.com/go-kit/log"
"go.uber.org/atomic"

"github.com/prometheus/common/promslog"
"github.com/prometheus/prometheus/model/labels"
"github.com/prometheus/prometheus/promql/parser"
"github.com/prometheus/prometheus/storage"
Expand All @@ -60,15 +61,15 @@ type writeBenchmark struct {
memprof *os.File
blockprof *os.File
mtxprof *os.File
logger log.Logger
logger *slog.Logger
}

func benchmarkWrite(outPath, samplesFile string, numMetrics, numScrapes int) error {
b := &writeBenchmark{
outPath: outPath,
samplesFile: samplesFile,
numMetrics: numMetrics,
logger: log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr)),
logger: promslog.New(&promslog.Config{}),
}
if b.outPath == "" {
dir, err := os.MkdirTemp("", "tsdb_bench")
Expand All @@ -87,9 +88,7 @@ func benchmarkWrite(outPath, samplesFile string, numMetrics, numScrapes int) err

dir := filepath.Join(b.outPath, "storage")

l := log.With(b.logger, "ts", log.DefaultTimestampUTC, "caller", log.DefaultCaller)

st, err := tsdb.Open(dir, l, nil, &tsdb.Options{
st, err := tsdb.Open(dir, b.logger, nil, &tsdb.Options{
RetentionDuration: int64(15 * 24 * time.Hour / time.Millisecond),
MinBlockDuration: int64(2 * time.Hour / time.Millisecond),
}, tsdb.NewDBStats())
Expand Down
4 changes: 2 additions & 2 deletions cmd/promtool/unittest.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ import (
"strings"
"time"

"github.com/go-kit/log"
"github.com/google/go-cmp/cmp"
"github.com/grafana/regexp"
"github.com/nsf/jsondiff"
"gopkg.in/yaml.v2"

"github.com/prometheus/common/model"
"github.com/prometheus/common/promslog"

"github.com/prometheus/prometheus/model/histogram"
"github.com/prometheus/prometheus/model/labels"
Expand Down Expand Up @@ -218,7 +218,7 @@ func (tg *testGroup) test(evalInterval time.Duration, groupOrderMap map[string]i
Appendable: suite.Storage(),
Context: context.Background(),
NotifyFunc: func(ctx context.Context, expr string, alerts ...*rules.Alert) {},
Logger: log.NewNopLogger(),
Logger: promslog.NewNopLogger(),
}
m := rules.NewManager(opts)
groupsMap, ers := m.LoadGroups(time.Duration(tg.Interval), tg.ExternalLabels, tg.ExternalURL, nil, ruleFiles...)
Expand Down
11 changes: 5 additions & 6 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package config
import (
"errors"
"fmt"
"log/slog"
"net/url"
"os"
"path/filepath"
Expand All @@ -25,8 +26,6 @@ import (
"time"

"github.com/alecthomas/units"
"github.com/go-kit/log"
"github.com/go-kit/log/level"
"github.com/grafana/regexp"
"github.com/prometheus/common/config"
"github.com/prometheus/common/model"
Expand Down Expand Up @@ -73,7 +72,7 @@ const (
)

// Load parses the YAML input s into a Config.
func Load(s string, expandExternalLabels bool, logger log.Logger) (*Config, error) {
func Load(s string, expandExternalLabels bool, logger *slog.Logger) (*Config, error) {
cfg := &Config{}
// If the entire config body is empty the UnmarshalYAML method is
// never called. We thus have to set the DefaultConfig at the entry
Expand All @@ -98,11 +97,11 @@ func Load(s string, expandExternalLabels bool, logger log.Logger) (*Config, erro
if v := os.Getenv(s); v != "" {
return v
}
level.Warn(logger).Log("msg", "Empty environment variable", "name", s)
logger.Warn("Empty environment variable", "name", s)
return ""
})
if newV != v.Value {
level.Debug(logger).Log("msg", "External label replaced", "label", v.Name, "input", v.Value, "output", newV)
logger.Debug("External label replaced", "label", v.Name, "input", v.Value, "output", newV)
}
// Note newV can be blank. https://github.com/prometheus/prometheus/issues/11024
b.Add(v.Name, newV)
Expand All @@ -112,7 +111,7 @@ func Load(s string, expandExternalLabels bool, logger log.Logger) (*Config, erro
}

// LoadFile parses the given YAML file into a Config.
func LoadFile(filename string, agentMode, expandExternalLabels bool, logger log.Logger) (*Config, error) {
func LoadFile(filename string, agentMode, expandExternalLabels bool, logger *slog.Logger) (*Config, error) {
content, err := os.ReadFile(filename)
if err != nil {
return nil, err
Expand Down
Loading

0 comments on commit b49314d

Please sign in to comment.