Skip to content

fix: Replace zerolog.Nop() with conditional logger #1352

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Replace zerolog.Nop() with conditional logger #1352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 13 additions & 13 deletions internal/checks/checks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestNewUpdater(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(make(chan pusher.Payload)),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
Features: testFeatureCollection,
},
},
Expand Down Expand Up @@ -168,7 +168,7 @@ func TestHandleCheckOp(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(publishCh),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
ScraperFactory: testScraperFactory,
},
)
Expand Down Expand Up @@ -285,7 +285,7 @@ func TestCheckHandlerProbeValidation(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(make(chan pusher.Payload)),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
K6Runner: noopRunner{},
},
probe: sm.Probe{Id: 100, Name: "test-probe", Capabilities: &sm.Probe_Capabilities{
Expand All @@ -300,7 +300,7 @@ func TestCheckHandlerProbeValidation(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(make(chan pusher.Payload)),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
},
probe: sm.Probe{Id: 100, Name: "test-probe", Capabilities: &sm.Probe_Capabilities{
DisableScriptedChecks: false,
Expand All @@ -314,7 +314,7 @@ func TestCheckHandlerProbeValidation(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(make(chan pusher.Payload)),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
K6Runner: noopRunner{},
},
probe: sm.Probe{Id: 100, Name: "test-probe", Capabilities: &sm.Probe_Capabilities{
Expand All @@ -329,7 +329,7 @@ func TestCheckHandlerProbeValidation(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(make(chan pusher.Payload)),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
},
probe: sm.Probe{Id: 100, Name: "test-probe", Capabilities: &sm.Probe_Capabilities{
DisableScriptedChecks: true,
Expand All @@ -343,7 +343,7 @@ func TestCheckHandlerProbeValidation(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(make(chan pusher.Payload)),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
K6Runner: noopRunner{},
},
probe: sm.Probe{Id: 100, Name: "test-probe", Capabilities: &sm.Probe_Capabilities{
Expand All @@ -358,7 +358,7 @@ func TestCheckHandlerProbeValidation(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(make(chan pusher.Payload)),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
},
probe: sm.Probe{Id: 100, Name: "test-probe", Capabilities: &sm.Probe_Capabilities{
DisableScriptedChecks: false,
Expand All @@ -372,7 +372,7 @@ func TestCheckHandlerProbeValidation(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(make(chan pusher.Payload)),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
K6Runner: noopRunner{},
},
probe: sm.Probe{Id: 100, Name: "test-probe", Capabilities: &sm.Probe_Capabilities{
Expand All @@ -387,7 +387,7 @@ func TestCheckHandlerProbeValidation(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(make(chan pusher.Payload)),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
},
probe: sm.Probe{Id: 100, Name: "test-probe", Capabilities: &sm.Probe_Capabilities{
DisableScriptedChecks: true,
Expand All @@ -401,7 +401,7 @@ func TestCheckHandlerProbeValidation(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(make(chan pusher.Payload)),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
},
probe: sm.Probe{Id: 100, Name: "test-probe"},
},
Expand All @@ -412,7 +412,7 @@ func TestCheckHandlerProbeValidation(t *testing.T) {
PromRegisterer: prometheus.NewPedanticRegistry(),
Publisher: channelPublisher(make(chan pusher.Payload)),
TenantCh: make(chan<- sm.Tenant),
Logger: zerolog.Nop(),
Logger: testhelper.Logger(t),
K6Runner: noopRunner{},
},
probe: sm.Probe{Id: 100, Name: "test-probe"},
Expand Down Expand Up @@ -524,7 +524,7 @@ func TestHandleError(t *testing.T) {
ctx, cancel := testhelper.Context(context.Background(), t)
defer cancel()

logger := zerolog.Nop()
logger := testhelper.Logger(t)

t.Run("no error", func(t *testing.T) {
backoff := testBackoff(1)
Expand Down
2 changes: 1 addition & 1 deletion internal/k6runner/http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ func TestScriptHTTPRun(t *testing.T) {
registry = prometheus.NewRegistry()
logBuf = bytes.Buffer{}
logger = recordingLogger{buf: &logBuf}
zlogger = zerolog.Nop()
zlogger = testhelper.Logger(t)
)

success, err := script.Run(ctx, registry, logger, zlogger, SecretStore{})
Expand Down
10 changes: 3 additions & 7 deletions internal/prober/multihttp/script_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import (
"context"
"net/http"
"net/http/httptest"
"path/filepath"
"runtime"
"strings"
"testing"

Expand All @@ -18,7 +16,6 @@ import (
sm "github.com/grafana/synthetic-monitoring-agent/pkg/pb/synthetic_monitoring"
"github.com/mccutchen/go-httpbin/v2/httpbin"
"github.com/prometheus/client_golang/prometheus"
"github.com/rs/zerolog"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -851,13 +848,12 @@ func TestSettingsToScript(t *testing.T) {

ctx, cancel := testhelper.Context(context.Background(), t)
t.Cleanup(cancel)
// logger := zerolog.New(zerolog.NewTestWriter(t))
logger := zerolog.Nop()
k6path := filepath.Join(testhelper.ModuleDir(t), "dist", runtime.GOOS+"-"+runtime.GOARCH, "sm-k6")
t.Log(k6path)

k6path := testhelper.K6Path(t)
runner := k6runner.New(k6runner.RunnerOpts{Uri: k6path})
store := noopSecretStore{}

logger := testhelper.Logger(t)
prober, err := NewProber(ctx, check, logger, runner, http.Header{}, &store)
require.NoError(t, err)

Expand Down
4 changes: 2 additions & 2 deletions internal/scraper/scraper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -960,7 +960,7 @@ func TestValidateLabels(t *testing.T) {
s := Scraper{
checkName: "check name",
target: check.Target,
logger: zerolog.Nop(),
logger: testhelper.Logger(t),
prober: prober,
labelsLimiter: testLabelsLimiter{
maxMetricLabels: 100,
Expand Down Expand Up @@ -1569,7 +1569,7 @@ func TestScraperCollectData(t *testing.T) {
s := Scraper{
checkName: checkName,
target: "test target",
logger: zerolog.Nop(),
logger: testhelper.Logger(t),
prober: testProber{},
labelsLimiter: testLabelsLimiter{
maxMetricLabels: tc.maxMetricLabels,
Expand Down
7 changes: 3 additions & 4 deletions internal/secrets/tenant_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@ import (
"errors"
"testing"

"github.com/rs/zerolog"

"github.com/grafana/synthetic-monitoring-agent/internal/model"
"github.com/grafana/synthetic-monitoring-agent/internal/testhelper"
sm "github.com/grafana/synthetic-monitoring-agent/pkg/pb/synthetic_monitoring"
"github.com/stretchr/testify/assert"
)
Expand All @@ -25,7 +24,7 @@ func TestGetSecretCredentials_Success(t *testing.T) {
mockSecretStore := &sm.SecretStore{}
mockTenant := sm.Tenant{SecretStore: mockSecretStore}
mockTenantProvider := &tenantProvider{tenant: mockTenant}
ts := NewTenantSecrets(mockTenantProvider, zerolog.Nop())
ts := NewTenantSecrets(mockTenantProvider, testhelper.Logger(t))
ctx := context.Background()
tenantID := model.GlobalID(1234)

Expand All @@ -38,7 +37,7 @@ func TestGetSecretCredentials_Success(t *testing.T) {
func TestGetSecretCredentials_Error(t *testing.T) {
getTenantErr := errors.New("tenant not found")
mockTenantProvider := &tenantProvider{err: getTenantErr}
ts := NewTenantSecrets(mockTenantProvider, zerolog.Nop())
ts := NewTenantSecrets(mockTenantProvider, testhelper.Logger(t))
ctx := context.Background()
tenantID := model.GlobalID(1234)

Expand Down
22 changes: 12 additions & 10 deletions internal/telemetry/region_pusher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import (
"testing"
"time"

"github.com/grafana/synthetic-monitoring-agent/internal/testhelper"
sm "github.com/grafana/synthetic-monitoring-agent/pkg/pb/synthetic_monitoring"

prom "github.com/prometheus/client_golang/prometheus"
prommodel "github.com/prometheus/client_model/go"
"github.com/rs/zerolog"
"github.com/stretchr/testify/require"
"google.golang.org/grpc"
)
Expand Down Expand Up @@ -228,7 +228,7 @@ func TestTenantPusher(t *testing.T) {
t.Run("should send telemetry data once", func(t *testing.T) {
t.Parallel()

td, tc, pusher, _ := setupTest(context.Background())
td, tc, pusher, _ := setupTest(t)

t.Cleanup(td.shutdownAndWait)

Expand All @@ -248,7 +248,7 @@ func TestTenantPusher(t *testing.T) {
t.Run("should retry sending data once", func(t *testing.T) {
t.Parallel()

td, tc, pusher, _ := setupTest(context.Background())
td, tc, pusher, _ := setupTest(t)

t.Cleanup(td.shutdownAndWait)

Expand All @@ -269,7 +269,7 @@ func TestTenantPusher(t *testing.T) {
t.Run("should retry and send more data", func(t *testing.T) {
t.Parallel()

td, tc, pusher, _ := setupTest(context.Background())
td, tc, pusher, _ := setupTest(t)

t.Cleanup(td.shutdownAndWait)

Expand Down Expand Up @@ -297,7 +297,7 @@ func TestTenantPusher(t *testing.T) {
t.Run("should push on context done", func(t *testing.T) {
t.Parallel()

td, tc, pusher, _ := setupTest(context.Background())
td, tc, pusher, _ := setupTest(t)

// Add some executions
addExecutions(pusher, getTestDataset(0).executions)
Expand Down Expand Up @@ -326,7 +326,7 @@ func TestTenantPusher(t *testing.T) {
t.Run("should report push error", func(t *testing.T) {
t.Parallel()

td, tc, pusher, metrics := setupTest(context.Background())
td, tc, pusher, metrics := setupTest(t)

t.Cleanup(td.shutdownAndWait)

Expand All @@ -350,7 +350,7 @@ func TestTenantPusher(t *testing.T) {
t.Run("should report push error on unexpected status", func(t *testing.T) {
t.Parallel()

td, tc, pusher, metrics := setupTest(context.Background())
td, tc, pusher, metrics := setupTest(t)

t.Cleanup(td.shutdownAndWait)

Expand Down Expand Up @@ -390,14 +390,14 @@ type testPushResp struct {
err error
}

func setupTest(ctx context.Context) (*testDriver, *testTelemetryClient, *RegionPusher, RegionMetrics) {
func setupTest(t *testing.T) (*testDriver, *testTelemetryClient, *RegionPusher, RegionMetrics) {
var (
wg = &sync.WaitGroup{}
testCtx, cancel = context.WithCancel(ctx)
testCtx, cancel = context.WithCancel(t.Context())
ticker = &testTicker{c: make(chan time.Time)}
td = testDriver{wg: wg, cancel: cancel, ticker: ticker}
tc = &testTelemetryClient{wg: wg}
logger = zerolog.Nop()
logger = testhelper.Logger(t)
metrics = RegionMetrics{
pushRequestsActive: prom.NewGauge(prom.GaugeOpts{}),
pushRequestsDuration: prom.NewHistogram(prom.HistogramOpts{}),
Expand All @@ -408,6 +408,8 @@ func setupTest(ctx context.Context) (*testDriver, *testTelemetryClient, *RegionP
opt withTicker = ticker
)

t.Cleanup(cancel)

pusher := NewRegionPusher(testCtx, 1*time.Second, tc, logger, instance, regionID, metrics, opt)

return &td, tc, pusher, metrics
Expand Down
24 changes: 12 additions & 12 deletions internal/telemetry/telemeter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import (
"testing"
"time"

"github.com/grafana/synthetic-monitoring-agent/internal/testhelper"
sm "github.com/grafana/synthetic-monitoring-agent/pkg/pb/synthetic_monitoring"
prom "github.com/prometheus/client_golang/prometheus"
"github.com/rs/zerolog"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -57,39 +57,39 @@ func TestTelemeterAddExecution(t *testing.T) {
registerer = prom.NewPedanticRegistry()
)

tele := NewTelemeter(ctx, instance, timeSpan, testClient, zerolog.Nop(), registerer)
tele := NewTelemeter(ctx, instance, timeSpan, testClient, testhelper.Logger(t), registerer)

t.Run("should init with no region pushers", func(t *testing.T) {
{ // should init with no region pushers
verifyTelemeter(t, tele, 0)
})
}

t.Run("should create a new region pusher", func(t *testing.T) {
{ // should create a new region pusher
execution := getTestDataset(0).executions[0]
tele.AddExecution(execution)
verifyTelemeter(t, tele, 1)
verifyRegionPusher(t, tele, execution.RegionID, execution)
})
}

t.Run("should add telemetry to current region pusher", func(t *testing.T) {
{ // should add telemetry to current region pusher
executions := getTestDataset(0).executions
tele.AddExecution(executions[1])
tele.AddExecution(executions[2])
verifyTelemeter(t, tele, 1)
verifyRegionPusher(t, tele, executions[0].RegionID, executions[:2]...)
})
}

t.Run("should add another region pusher", func(t *testing.T) {
{ // should add another region pusher
executions := getTestDataset(0).executions
executions[2].RegionID = 1
tele.AddExecution(executions[2])
executions[3].RegionID = 1
tele.AddExecution(executions[3])
verifyTelemeter(t, tele, 2)
verifyRegionPusher(t, tele, executions[3].RegionID, executions[2:4]...)
})
}

t.Run("initial region pusher data should be intact", func(t *testing.T) {
{ // initial region pusher data should be intact
executions := getTestDataset(0).executions
verifyRegionPusher(t, tele, executions[0].RegionID, executions[:2]...)
})
}
}
19 changes: 19 additions & 0 deletions internal/testhelper/testhelper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"
"time"

"github.com/rs/zerolog"
"github.com/stretchr/testify/require"
)

Expand All @@ -20,6 +21,15 @@ func Context(ctx context.Context, t *testing.T) (context.Context, context.Cancel
return context.WithDeadline(ctx, deadline)
}

func Logger(t *testing.T) zerolog.Logger {
logger := zerolog.New(zerolog.NewTestWriter(t)).Level(zerolog.ErrorLevel)
if testing.Verbose() {
logger = logger.Level(zerolog.DebugLevel)
}

return logger.With().Caller().Timestamp().Logger()
}

func MustReadFile(t *testing.T, filename string) []byte {
t.Helper()

Expand Down Expand Up @@ -55,3 +65,12 @@ func ModuleDir(t *testing.T) string {

return dir
}

func K6Path(t *testing.T) string {
t.Helper()

k6path := filepath.Join(ModuleDir(t), "dist", runtime.GOOS+"-"+runtime.GOARCH, "sm-k6")
require.FileExistsf(t, k6path, "k6 program must exist at %s", k6path)

return k6path
}
Loading