From 4ed33017a588091da620710321847638dfce23d1 Mon Sep 17 00:00:00 2001 From: Silvano Ravotto Date: Tue, 15 Sep 2026 16:37:56 -0400 Subject: [PATCH 1/8] test: add coverage for the "init" command and Store.Init idempotency internal/cmd/initialize had zero test coverage, and no test verified that re-running the schema-init DDL is a safe no-op, even though every statement in ddl.sql relies on IF NOT EXISTS for that guarantee. Co-Authored-By: roachdev-claude --- .../cmd/initialize/admin_integration_test.go | 77 +++++++++++++++++++ internal/cmd/initialize/admin_test.go | 28 +++++++ internal/store/init_test.go | 39 ++++++++++ 3 files changed, 144 insertions(+) create mode 100644 internal/cmd/initialize/admin_integration_test.go create mode 100644 internal/cmd/initialize/admin_test.go create mode 100644 internal/store/init_test.go diff --git a/internal/cmd/initialize/admin_integration_test.go b/internal/cmd/initialize/admin_integration_test.go new file mode 100644 index 0000000..88e9726 --- /dev/null +++ b/internal/cmd/initialize/admin_integration_test.go @@ -0,0 +1,77 @@ +// Copyright 2026 Cockroach Labs Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build integration + +package initialize + +import ( + "context" + "io" + "os" + "testing" + "time" + + "github.com/cockroachlabs/visus/internal/testutil" + "github.com/stretchr/testify/require" +) + +// TestMain starts the shared CockroachDB cluster used by every integration +// test in this package. +func TestMain(m *testing.M) { + os.Exit(testutil.StartCluster(m)) +} + +// captureStdout runs f with os.Stdout redirected to a pipe and returns +// whatever f wrote to it. admin.go prints its result with fmt.Printf +// directly to os.Stdout rather than through the cobra command's output +// writer, so cmd.SetOut has nothing to capture. +func captureStdout(t *testing.T, f func()) string { + t.Helper() + r := require.New(t) + + pipeReader, pipeWriter, err := os.Pipe() + r.NoError(err) + orig := os.Stdout + os.Stdout = pipeWriter + defer func() { os.Stdout = orig }() + + f() + + r.NoError(pipeWriter.Close()) + out, err := io.ReadAll(pipeReader) + r.NoError(err) + return string(out) +} + +// TestInitCommandIdempotent verifies that running the "init" CLI command +// twice against the same database succeeds both times, since it's the +// upgrade path an operator runs on every restart. +func TestInitCommandIdempotent(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + r := require.New(t) + + url := testutil.PGURL().String() + + for range 2 { + cmd := Command() + cmd.SetContext(ctx) + cmd.SetArgs([]string{"--url", url}) + out := captureStdout(t, func() { + r.NoError(cmd.Execute()) + }) + r.Contains(out, "Database initialized at") + } +} diff --git a/internal/cmd/initialize/admin_test.go b/internal/cmd/initialize/admin_test.go new file mode 100644 index 0000000..11891eb --- /dev/null +++ b/internal/cmd/initialize/admin_test.go @@ -0,0 +1,28 @@ +// Copyright 2026 Cockroach Labs Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package initialize + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestHelp ensures that the CLI command can be constructed and +// that all flag binding works. +func TestHelp(t *testing.T) { + r := require.New(t) + r.NoError(Command().Help()) +} diff --git a/internal/store/init_test.go b/internal/store/init_test.go new file mode 100644 index 0000000..3b1b507 --- /dev/null +++ b/internal/store/init_test.go @@ -0,0 +1,39 @@ +// Copyright 2026 Cockroach Labs Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build integration + +package store_test + +import ( + "context" + "testing" + "time" + + "github.com/cockroachlabs/visus/internal/testutil" + "github.com/stretchr/testify/require" +) + +// TestInitIdempotent verifies that Init can be called repeatedly against a +// schema it already created, since the schema-upgrade path relies on +// re-running the same DDL on every startup. +func TestInitIdempotent(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + r := require.New(t) + st, _ := testutil.NewStore(ctx, t) // Init already ran once inside NewStore + + r.NoError(st.Init(ctx)) + r.NoError(st.Init(ctx)) +} From 193cde44f5c37d82f42bf50eff21e3da5e651b4c Mon Sep 17 00:00:00 2001 From: Silvano Ravotto Date: Tue, 15 Sep 2026 16:38:04 -0400 Subject: [PATCH 2/8] test: add coverage for the "start" command's flags and node lifecycle internal/cmd/server had zero test coverage. This adds a unit test for the two flag-consistency checks in RunE (no cluster needed, since they return before any connection is attempted) and an integration test verifying that the server registers its node on startup and deregisters it on shutdown. The node store methods themselves are already covered by internal/store/node_test.go; this only exercises server.go's wiring of them. Co-Authored-By: roachdev-claude --- .../cmd/server/server_integration_test.go | 84 +++++++++++++++++++ internal/cmd/server/server_test.go | 61 ++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 internal/cmd/server/server_integration_test.go create mode 100644 internal/cmd/server/server_test.go diff --git a/internal/cmd/server/server_integration_test.go b/internal/cmd/server/server_integration_test.go new file mode 100644 index 0000000..b01e824 --- /dev/null +++ b/internal/cmd/server/server_integration_test.go @@ -0,0 +1,84 @@ +// Copyright 2026 Cockroach Labs Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build integration + +package server + +import ( + "context" + "os" + "testing" + "time" + + "github.com/cockroachlabs/visus/internal/testutil" + "github.com/stretchr/testify/require" +) + +// TestMain starts the shared CockroachDB cluster used by every integration +// test in this package. +func TestMain(m *testing.M) { + os.Exit(testutil.StartCluster(m)) +} + +// TestServerNodeLifecycle verifies that RunE wires the node +// registration/heartbeat/deregistration calls correctly: the node appears +// in the store once the server starts, and disappears once the server's +// context is canceled. The store methods themselves (RegisterNode, +// Heartbeat, DeleteNode) are already covered by internal/store/node_test.go; +// this test only exercises server.go's use of them. +func TestServerNodeLifecycle(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + r := require.New(t) + + // Command()'s RunE does not initialize the schema itself; it expects + // "visus init" to have already run. testutil.NewStore does that for us + // and also gives us a Store we can poll from outside the server. + st, _ := testutil.NewStore(ctx, t) + + runCtx, stopServer := context.WithCancel(ctx) + defer stopServer() + + cmd := Command() + cmd.SetContext(runCtx) + cmd.SetArgs([]string{ + "--insecure", + "--bind-addr", "127.0.0.1:0", + "--endpoint", "/test/node-lifecycle", + "--url", testutil.PGURL().String(), + }) + + done := make(chan error, 1) + go func() { done <- cmd.Execute() }() + + r.Eventually(func() bool { + nodes, err := st.GetNodes(ctx) + return err == nil && len(nodes) == 1 + }, 15*time.Second, 200*time.Millisecond, "server did not register its node") + + stopServer() + + r.Eventually(func() bool { + nodes, err := st.GetNodes(ctx) + return err == nil && len(nodes) == 0 + }, 15*time.Second, 200*time.Millisecond, "server did not deregister its node on shutdown") + + select { + case err := <-done: + r.NoError(err) + case <-time.After(15 * time.Second): + t.Fatal("Command().Execute() did not return after shutdown") + } +} diff --git a/internal/cmd/server/server_test.go b/internal/cmd/server/server_test.go new file mode 100644 index 0000000..fc2b9ca --- /dev/null +++ b/internal/cmd/server/server_test.go @@ -0,0 +1,61 @@ +// Copyright 2026 Cockroach Labs Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package server + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestHelp ensures that the CLI command can be constructed and +// that all flag binding works. +func TestHelp(t *testing.T) { + r := require.New(t) + r.NoError(Command().Help()) +} + +// TestFlagValidation verifies that the two flag-consistency checks at the +// top of RunE fire before any database connection is attempted, since both +// return before dialing the URL. +func TestFlagValidation(t *testing.T) { + tests := []struct { + name string + args []string + wantErr string + }{ + { + name: "missing certs without insecure", + args: []string{}, + wantErr: "--insecure must be specfied if certificates and private key are missing", + }, + { + name: "missing url", + args: []string{"--insecure"}, + wantErr: "--url must be specified", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := assert.New(t) + cmd := Command() + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs(tt.args) + a.ErrorContains(cmd.Execute(), tt.wantErr) + }) + } +} From 2691b00b8fe89a3f47c8b6b6d59b8923074f0077 Mon Sep 17 00:00:00 2001 From: Silvano Ravotto Date: Tue, 15 Sep 2026 16:38:20 -0400 Subject: [PATCH 3/8] test: exercise the malformed-JSON error path in parseAuthLine TestParseAuthLine had a wantErr field that no case ever set, and the loop body ran a.NoError(err) unconditionally, so setting wantErr on a future case would not even have worked. Add a truncated-JSON case and short-circuit the loop when wantErr is set. Co-Authored-By: roachdev-claude --- internal/scanner/crdbv2auth_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/scanner/crdbv2auth_test.go b/internal/scanner/crdbv2auth_test.go index 8be0e35..bf117f0 100644 --- a/internal/scanner/crdbv2auth_test.go +++ b/internal/scanner/crdbv2auth_test.go @@ -71,6 +71,12 @@ func TestParseAuthLine(t *testing.T) { }, "", }, + { + "truncated_json", + `I250407 20:22:58.997928 731936 4@util/log/event_log.go:39 ⋮ [T1,Vsystem,n1,client=127.0.0.1:53480,hostssl,user=‹roachprod›] 15 ={"Timestamp":1744057378997924809,"EventType":"client_authentication_ok"`, + nil, + "unexpected end of JSON input", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -78,6 +84,7 @@ func TestParseAuthLine(t *testing.T) { res, err := parseAuthLine([]byte(tt.line)) if tt.wantErr != "" { a.EqualError(err, tt.wantErr) + return } a.NoError(err) a.Equal(tt.want, res) From eea5e0b43155f8b9201f0a7fef85acb3f44510c2 Mon Sep 17 00:00:00 2001 From: Silvano Ravotto Date: Tue, 15 Sep 2026 16:38:30 -0400 Subject: [PATCH 4/8] scan: validate Format at YAML parse time Format was cast straight from the YAML string with no allow-list, unlike Scope in the collection config. An invalid value only surfaced later inside scanner.FromConfig, which scannerServer.refresh logs and skips, so a bad Format silently dropped the scan instead of failing the upload. Mirror the Scope validation pattern to reject it at unmarshal time, and add a test in server_test.go that locks in the scanner-layer skip as a safety net for values that reach the store by some other path. The scan2 fixture in admin_test.go needed a valid Format since it now fails validation on the round trip through the "put" command. Co-Authored-By: roachdev-claude --- internal/cmd/scan/admin_test.go | 2 +- .../cmd/scan/testdata/invalid_format.yaml | 18 +++ internal/cmd/scan/testdata/malformed.yaml | 18 +++ internal/cmd/scan/yaml.go | 13 +- internal/cmd/scan/yaml_test.go | 119 ++++++++++++++++++ internal/scanner/server_test.go | 39 ++++++ 6 files changed, 207 insertions(+), 2 deletions(-) create mode 100644 internal/cmd/scan/testdata/invalid_format.yaml create mode 100644 internal/cmd/scan/testdata/malformed.yaml create mode 100644 internal/cmd/scan/yaml_test.go diff --git a/internal/cmd/scan/admin_test.go b/internal/cmd/scan/admin_test.go index 1393d15..8593244 100644 --- a/internal/cmd/scan/admin_test.go +++ b/internal/cmd/scan/admin_test.go @@ -94,7 +94,7 @@ func TestCommands(t *testing.T) { mscan1, err := marshal(scan1) r.NoError(err) scan1Cfg := string(mscan1) - scan2 := &store.Scan{Name: "scan_02"} + scan2 := &store.Scan{Name: "scan_02", Format: store.CRDBv2} mscan2, err := marshal(scan2) r.NoError(err) scan2Cfg := string(mscan2) diff --git a/internal/cmd/scan/testdata/invalid_format.yaml b/internal/cmd/scan/testdata/invalid_format.yaml new file mode 100644 index 0000000..26ef585 --- /dev/null +++ b/internal/cmd/scan/testdata/invalid_format.yaml @@ -0,0 +1,18 @@ +# Copyright 2026 Cockroach Labs Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: scan_bad_format +enabled: true +format: bogus +path: testdata/sample.log diff --git a/internal/cmd/scan/testdata/malformed.yaml b/internal/cmd/scan/testdata/malformed.yaml new file mode 100644 index 0000000..c78d5b8 --- /dev/null +++ b/internal/cmd/scan/testdata/malformed.yaml @@ -0,0 +1,18 @@ +# Copyright 2026 Cockroach Labs Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: scan_malformed +enabled: notabool +format: crdb-v2 +path: testdata/sample.log diff --git a/internal/cmd/scan/yaml.go b/internal/cmd/scan/yaml.go index 49b3c2a..44f79ec 100644 --- a/internal/cmd/scan/yaml.go +++ b/internal/cmd/scan/yaml.go @@ -15,6 +15,8 @@ package scan import ( + "strings" + "github.com/cockroachdb/errors" "github.com/cockroachlabs/visus/internal/store" "github.com/creasty/defaults" @@ -81,9 +83,18 @@ func unmarshal(data []byte) (*store.Scan, error) { if config.Name == "" { return nil, errors.New("name must be specified") } + var format store.LogFormat + switch strings.ToLower(config.Format) { + case string(store.CRDBv2): + format = store.CRDBv2 + case string(store.CRDBv2Auth): + format = store.CRDBv2Auth + default: + return nil, errors.New("invalid format") + } return &store.Scan{ Enabled: config.Enabled, - Format: store.LogFormat(config.Format), + Format: format, Path: config.Path, Name: config.Name, Patterns: patterns, diff --git a/internal/cmd/scan/yaml_test.go b/internal/cmd/scan/yaml_test.go new file mode 100644 index 0000000..5c00e4d --- /dev/null +++ b/internal/cmd/scan/yaml_test.go @@ -0,0 +1,119 @@ +// Copyright 2026 Cockroach Labs Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package scan + +import ( + _ "embed" + "testing" + + "github.com/cockroachlabs/visus/internal/store" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +//go:embed testdata/invalid_format.yaml +var invalidFormat string + +//go:embed testdata/malformed.yaml +var malformed string + +// TestMarshalRoundTrip verifies we can marshal/unmarshal a scan configuration. +func TestMarshalRoundTrip(t *testing.T) { + tests := []struct { + name string + scan *store.Scan + yaml string + wantErr string + }{ + { + name: "good crdb-v2", + scan: &store.Scan{ + Enabled: true, + Format: store.CRDBv2, + Name: "scan_01", + Path: "testdata/sample.log", + Patterns: []store.Pattern{ + { + Help: "description", + Name: "metric", + }, + }, + }, + }, + { + name: "good crdb-v2-auth", + scan: &store.Scan{ + Enabled: true, + Format: store.CRDBv2Auth, + Name: "scan_02", + Path: "testdata/auth.log", + Patterns: []store.Pattern{ + { + Help: "auth attempts", + Name: "auth", + }, + }, + }, + }, + { + name: "no name", + scan: &store.Scan{ + Enabled: true, + Format: store.CRDBv2, + Patterns: []store.Pattern{ + { + Help: "description", + Name: "metric", + }, + }, + }, + wantErr: "name must be specified", + }, + { + name: "invalid format", + yaml: invalidFormat, + wantErr: "invalid format", + }, + { + name: "unmarshal error", + yaml: malformed, + wantErr: "cannot unmarshal", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + r := require.New(t) + a := assert.New(t) + var data []byte + var err error + switch { + case test.scan != nil: + data, err = marshal(test.scan) + r.NoError(err) + case test.yaml != "": + data = []byte(test.yaml) + default: + panic("invalid test case") + } + out, err := unmarshal(data) + if test.wantErr != "" { + a.ErrorContains(err, test.wantErr) + return + } + r.NoError(err) + a.Equal(test.scan, out) + }) + } +} diff --git a/internal/scanner/server_test.go b/internal/scanner/server_test.go index f7a0818..88a93d6 100644 --- a/internal/scanner/server_test.go +++ b/internal/scanner/server_test.go @@ -171,6 +171,45 @@ func TestRefreshScanners(t *testing.T) { } +// TestRefreshScannersInvalidFormat verifies that a scan with an unsupported +// Format is skipped rather than failing the whole refresh. This is the +// server-layer safety net that complements the fail-fast Format validation +// added at YAML-parse time in internal/cmd/scan/yaml.go: a Format value that +// reaches the store through some other path (e.g. a stale row from a schema +// version that supported it) should not block valid scans from starting. +func TestRefreshScannersInvalidFormat(t *testing.T) { + r := require.New(t) + a := assert.New(t) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + stopperCtx := stopper.WithContext(ctx) + mockStore := &store.Memory{} + mockStore.Init(ctx) + cfg := &server.Config{} + registry := prometheus.NewRegistry() + scanners := &scannerServer{ + config: cfg, + fromBeginning: true, + registry: registry, + store: mockStore, + } + scanners.mu.scanners = make(map[string]*Scanner) + + badScan := &store.Scan{ + Enabled: true, + Format: store.LogFormat("bogus"), + Name: "badformat", + Path: "./testdata/sample.log", + } + mockStore.PutScan(ctx, badScan) + + r.NoError(scanners.Refresh(stopperCtx)) + + _, ok := scanners.get(badScan.Name) + a.False(ok, "scan with an unsupported format should not have been registered") +} + func waitForMetrics(ctx context.Context, registry *prometheus.Registry, expected int) error { for { metrics, err := registry.Gather() From c0f7a64859cdea2b7738c6c3e638d43cdf4fe209 Mon Sep 17 00:00:00 2001 From: Silvano Ravotto Date: Tue, 15 Sep 2026 16:38:46 -0400 Subject: [PATCH 5/8] collection: validate metric Kind and duplicate Labels; fail loudly in FromCollection yaml.go validated Scope against an allow-list but never validated metric Kind, and never checked for duplicate Labels entries. Add both checks at unmarshal time, mirroring the existing Scope pattern. Separately, the Kind switch in FromCollection only logged on an unrecognized Kind without setting the returned error, so a bad Kind that reached the database through some path other than this YAML loader would be silently dropped instead of failing collector construction. Fix it to return an error, and add a test that could not otherwise assert correct behavior. Co-Authored-By: roachdev-claude --- .../collection/testdata/duplicate_labels.yaml | 37 ++++++++++++++++++ .../cmd/collection/testdata/invalid_kind.yaml | 38 +++++++++++++++++++ internal/cmd/collection/yaml.go | 14 ++++++- internal/cmd/collection/yaml_test.go | 16 ++++++++ internal/collector/collector.go | 2 +- internal/collector/collector_test.go | 19 ++++++++++ 6 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 internal/cmd/collection/testdata/duplicate_labels.yaml create mode 100644 internal/cmd/collection/testdata/invalid_kind.yaml diff --git a/internal/cmd/collection/testdata/duplicate_labels.yaml b/internal/cmd/collection/testdata/duplicate_labels.yaml new file mode 100644 index 0000000..019bdfc --- /dev/null +++ b/internal/cmd/collection/testdata/duplicate_labels.yaml @@ -0,0 +1,37 @@ +# Copyright 2026 Cockroach Labs Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: query_count +enabled: true +scope: node +frequency: 10 +maxresults: 100 +labels: [database,database] +metrics: + - name : exec_count + kind : counter + help : statement count per application and database. +query: + SELECT + database_name as database, + sum(count) AS exec_count + FROM + crdb_internal.node_statement_statistics + WHERE + application_name NOT LIKE '$ internal-%' + GROUP BY + database_name + ORDER BY + exec_count DESC + LIMIT $1; diff --git a/internal/cmd/collection/testdata/invalid_kind.yaml b/internal/cmd/collection/testdata/invalid_kind.yaml new file mode 100644 index 0000000..3ad1bd7 --- /dev/null +++ b/internal/cmd/collection/testdata/invalid_kind.yaml @@ -0,0 +1,38 @@ +# Copyright 2026 Cockroach Labs Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: query_count +enabled: true +scope: node +frequency: 10 +maxresults: 100 +labels: [application,database] +metrics: + - name : exec_count + kind : bogus + help : statement count per application and database. +query: + SELECT + application_name as application, + database_name as database, + sum(count) AS exec_count + FROM + crdb_internal.node_statement_statistics + WHERE + application_name NOT LIKE '$ internal-%' + GROUP BY + application_name, database_name + ORDER BY + exec_count DESC + LIMIT $1; diff --git a/internal/cmd/collection/yaml.go b/internal/cmd/collection/yaml.go index a57763b..5b27718 100644 --- a/internal/cmd/collection/yaml.go +++ b/internal/cmd/collection/yaml.go @@ -80,12 +80,24 @@ func unmarshal(data []byte) (*store.Collection, error) { } metrics := make([]store.Metric, 0) for _, m := range config.Metrics { + switch strings.ToLower(m.Kind) { + case string(store.Gauge), string(store.Counter): + default: + return nil, errors.New("invalid kind") + } metrics = append(metrics, store.Metric{ Name: m.Name, - Kind: store.Kind(m.Kind), + Kind: store.Kind(strings.ToLower(m.Kind)), Help: m.Help, }) } + seenLabels := make(map[string]struct{}, len(config.Labels)) + for _, l := range config.Labels { + if _, ok := seenLabels[l]; ok { + return nil, errors.Errorf("duplicate label %q", l) + } + seenLabels[l] = struct{}{} + } var scope store.Scope switch strings.ToLower(config.Scope) { case "node": diff --git a/internal/cmd/collection/yaml_test.go b/internal/cmd/collection/yaml_test.go index 97ce05d..9e3dd6f 100644 --- a/internal/cmd/collection/yaml_test.go +++ b/internal/cmd/collection/yaml_test.go @@ -27,6 +27,12 @@ import ( //go:embed testdata/invalid_scope.yaml var invalidScope string +//go:embed testdata/invalid_kind.yaml +var invalidKind string + +//go:embed testdata/duplicate_labels.yaml +var duplicateLabels string + //go:embed testdata/malformed.yaml var malformed string @@ -97,6 +103,16 @@ func TestMarshalRoundTrip(t *testing.T) { yaml: invalidScope, wantErr: "invalid scope", }, + { + name: "invalid kind", + yaml: invalidKind, + wantErr: "invalid kind", + }, + { + name: "duplicate labels", + yaml: duplicateLabels, + wantErr: "duplicate label", + }, { name: "unmarshal error", yaml: malformed, diff --git a/internal/collector/collector.go b/internal/collector/collector.go index 478a362..f77dc37 100644 --- a/internal/collector/collector.go +++ b/internal/collector/collector.go @@ -141,7 +141,7 @@ func FromCollection(coll *store.Collection, registerer prometheus.Registerer) (C case store.Counter: err = res.AddCounter(m.Name, m.Help) default: - log.Errorf("%s malformed", coll.Name) + err = errors.Errorf("%s: unsupported metric kind %q for metric %s", coll.Name, m.Kind, m.Name) } if err != nil { return nil, err diff --git a/internal/collector/collector_test.go b/internal/collector/collector_test.go index 009673f..28b80c2 100644 --- a/internal/collector/collector_test.go +++ b/internal/collector/collector_test.go @@ -20,6 +20,7 @@ import ( "strings" "testing" + "github.com/cockroachlabs/visus/internal/store" "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" "github.com/stretchr/testify/assert" @@ -270,6 +271,24 @@ func TestCollectSkipsConcurrent(t *testing.T) { c.mu.Unlock() } +// TestFromCollectionUnsupportedKind verifies that FromCollection fails +// instead of silently dropping a metric whose Kind isn't gauge or counter. +func TestFromCollectionUnsupportedKind(t *testing.T) { + _, r := assertions(t) + coll := &store.Collection{ + Name: "bad_kind", + Metrics: []store.Metric{ + { + Name: "metric", + Kind: store.Kind("bogus"), + Help: "help", + }, + }, + } + _, err := FromCollection(coll, prometheus.NewRegistry()) + r.Error(err) +} + func TestCounterLifeCycle(t *testing.T) { a, r := assertions(t) collName := "lifecycle" From 82f787a12201584260470897cd8f3fcdc90e00cf Mon Sep 17 00:00:00 2001 From: Silvano Ravotto Date: Tue, 15 Sep 2026 16:52:45 -0400 Subject: [PATCH 6/8] database: verify Refresh replaces and closes the old pool Refresh is the SIGHUP path for picking up rotated certificates, but nothing confirmed it actually swapped and closed the old pgxpool.Pool rather than merely reconnecting in place. The test needs white-box access to the underlying pgxpool.Pool inside Pool, which the existing database_test integration tests cannot get without an import cycle through internal/testutil, so it uses the standard export_test.go pattern to bridge the two. Co-Authored-By: roachdev-claude --- internal/database/export_test.go | 28 ++++++++++++ internal/database/pool_integration_test.go | 52 ++++++++++++++++++++++ 2 files changed, 80 insertions(+) create mode 100644 internal/database/export_test.go create mode 100644 internal/database/pool_integration_test.go diff --git a/internal/database/export_test.go b/internal/database/export_test.go new file mode 100644 index 0000000..338385e --- /dev/null +++ b/internal/database/export_test.go @@ -0,0 +1,28 @@ +// Copyright 2026 Cockroach Labs Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// This file exposes package-private state to tests in database_test. It +// cannot import internal/testutil itself: testutil imports database, and +// this file's package declaration (database, not database_test) would +// create an import cycle if it did. +package database + +import "github.com/jackc/pgx/v5/pgxpool" + +// PoolUnderlying returns the pgxpool.Pool a Pool currently delegates to, so +// tests can confirm Refresh actually swaps and closes it rather than just +// observing that queries keep working afterward. +func PoolUnderlying(p *Pool) *pgxpool.Pool { + return p.get() +} diff --git a/internal/database/pool_integration_test.go b/internal/database/pool_integration_test.go new file mode 100644 index 0000000..bf77c1a --- /dev/null +++ b/internal/database/pool_integration_test.go @@ -0,0 +1,52 @@ +// Copyright 2026 Cockroach Labs Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build integration + +package database_test + +import ( + "context" + "testing" + "time" + + "github.com/cockroachlabs/visus/internal/database" + "github.com/cockroachlabs/visus/internal/testutil" + "github.com/stretchr/testify/require" +) + +// TestPoolRefresh verifies that Refresh installs a new underlying pool and +// closes the old one, rather than merely reconnecting in place. This is the +// path exercised on SIGHUP to pick up rotated certificates. +func TestPoolRefresh(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + r := require.New(t) + + pool, err := database.New(ctx, testutil.PGURL().String()) + r.NoError(err) + + oldPgxPool := database.PoolUnderlying(pool) + r.NoError(oldPgxPool.Ping(ctx)) + + r.NoError(pool.Refresh(ctx)) + + newPgxPool := database.PoolUnderlying(pool) + r.NotSame(oldPgxPool, newPgxPool, "Refresh should install a new underlying pool") + r.Error(oldPgxPool.Ping(ctx), "Refresh should close the old pool") + + var one int + r.NoError(pool.QueryRow(ctx, "SELECT 1").Scan(&one)) + r.Equal(1, one) +} From e910524bf860a610bb7788f925d65d846946f279 Mon Sep 17 00:00:00 2001 From: Silvano Ravotto Date: Tue, 15 Sep 2026 16:52:55 -0400 Subject: [PATCH 7/8] scanner: verify lines without the CRDB log prefix are skipped, not miscounted Real CockroachDB logs interleave lines that do not start with the [FEIW]yymmdd prefix, most commonly a panic stack trace. Every existing fixture happened to have that prefix on every line, so the early return in CRDBv2Parser.Parse and CRDBv2AuthParser.Parse for non-matching lines was never exercised through the full scan/parse pipeline. Add a fixture that mixes valid and invalid lines and confirm only the valid ones are counted. Co-Authored-By: roachdev-claude --- internal/scanner/scanner_test.go | 26 +++++++++++++++++++++++ internal/scanner/testdata/nonmatching.log | 6 ++++++ internal/scanner/testdata/nonmatching.txt | 4 ++++ 3 files changed, 36 insertions(+) create mode 100644 internal/scanner/testdata/nonmatching.log create mode 100644 internal/scanner/testdata/nonmatching.txt diff --git a/internal/scanner/scanner_test.go b/internal/scanner/scanner_test.go index 61a80e3..e36e5c9 100644 --- a/internal/scanner/scanner_test.go +++ b/internal/scanner/scanner_test.go @@ -57,6 +57,9 @@ var maxExpected string //go:embed testdata/cache.txt var cacheExpected string +//go:embed testdata/nonmatching.txt +var nonmatchingExpected string + // TestScanner verifies we can produce metrics from a test file. func TestScanner(t *testing.T) { tests := []struct { @@ -166,6 +169,29 @@ func TestScanner(t *testing.T) { "crdb_auth": authExpected, }, }, + { + // Real CockroachDB logs interleave lines that do not start with + // the [FEIW]yymmdd prefix, most commonly a panic stack trace. + // This verifies those lines are silently skipped rather than + // miscounted or treated as parse errors. + "non-matching prefix lines are skipped", + &store.Scan{ + Enabled: true, + Format: store.CRDBv2, + Name: "crdb", + Path: "./testdata/nonmatching.log", + Patterns: []store.Pattern{ + { + Name: "all", + Regex: "", + Help: "all events", + }, + }, + }, + map[string]string{ + "crdb_all": nonmatchingExpected, + }, + }, } for _, tt := range tests { diff --git a/internal/scanner/testdata/nonmatching.log b/internal/scanner/testdata/nonmatching.log new file mode 100644 index 0000000..a00ebd4 --- /dev/null +++ b/internal/scanner/testdata/nonmatching.log @@ -0,0 +1,6 @@ +I240418 15:31:56.563605 1 1@cli/start.go:1316 ⋮ [T1,n?] 8 using local environment variables: +panic: something went wrong +goroutine 1 [running]: +main.main() + /app/main.go:42 +0x1a +I240418 15:31:56.801319 97 server/config.go:860 ⋮ [T1,n?] 12 1 storage engine initialized diff --git a/internal/scanner/testdata/nonmatching.txt b/internal/scanner/testdata/nonmatching.txt new file mode 100644 index 0000000..d7d8db8 --- /dev/null +++ b/internal/scanner/testdata/nonmatching.txt @@ -0,0 +1,4 @@ +# HELP crdb_all all events +# TYPE crdb_all counter +crdb_all{level="I",source="cli/start.go"} 1 +crdb_all{level="I",source="server/config.go"} 1 From 18d12b5cf5cbe58cd93a328aa290c02a82a27958 Mon Sep 17 00:00:00 2001 From: Silvano Ravotto Date: Tue, 15 Sep 2026 16:53:16 -0400 Subject: [PATCH 8/8] test: detect goroutine leaks in scanner and lease lifecycle tests Add go.uber.org/goleak (already a transitive dependency, now direct) and check for leaked goroutines after TestRefreshScanners stops its scanners and after TestLeaseLifecycle tears down both servers. These are exactly the concurrent Start/Stop paths a leak would otherwise slip through unnoticed. Two categories of goroutines needed explicit ignore rules, both started by third-party libraries with no way for this code to close them: nxadm/tail lazily starts a single process-wide file-watcher pair reused by every Tail for the life of the process, and database.Pool has no Close method since its pools are designed to live for the process lifetime too. Neither is a leak in the code these tests exist to verify. TestLeaseLifecycle registers its own check via t.Cleanup rather than a plain defer, since t.Cleanup callbacks run after a test function's own defers: a plain defer would have run before newTestServer's scheduler.Stop cleanups and flagged their still-running executor goroutines as leaks. Co-Authored-By: roachdev-claude --- go.mod | 1 + internal/collector/lease_test.go | 28 ++++++++++++++++++++++++++++ internal/scanner/server_test.go | 16 ++++++++++++++++ 3 files changed, 45 insertions(+) diff --git a/go.mod b/go.mod index a98896d..a195dd0 100644 --- a/go.mod +++ b/go.mod @@ -22,6 +22,7 @@ require ( github.com/sirupsen/logrus v1.10.2 github.com/spf13/cobra v1.10.2 github.com/stretchr/testify v1.12.1 + go.uber.org/goleak v1.3.0 golang.org/x/lint v0.0.0-20210508222113-6edffad5e616 golang.org/x/tools v0.50.0 google.golang.org/protobuf v1.36.12 diff --git a/internal/collector/lease_test.go b/internal/collector/lease_test.go index 3c288fd..3409a47 100644 --- a/internal/collector/lease_test.go +++ b/internal/collector/lease_test.go @@ -33,6 +33,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) // testSetup resets the shared cluster's schema and returns its pg URL. @@ -118,6 +119,25 @@ func clusterCollection(name string) *store.Collection { func TestLeaseLifecycle(t *testing.T) { r := require.New(t) a := assert.New(t) + // The baseline must be captured now, before any servers start, but the + // check itself must run after newTestServer's t.Cleanup(scheduler.Stop) + // calls below: t.Cleanup callbacks run after this function's own defers, + // so a plain "defer goleak.VerifyNone" would fire first and flag each + // scheduler's still-running executor goroutine as a leak. Registering + // our own t.Cleanup here, before newTestServer registers its own, makes + // it run last (t.Cleanup is LIFO), after both schedulers have stopped. + ignoreBaseline := goleak.IgnoreCurrent() + t.Cleanup(func() { + goleak.VerifyNone(t, + ignoreBaseline, + // database.Pool has no Close method: its pools are designed to + // live for the process lifetime, so this test cannot release + // the admin/read-only connections it opens below either. That + // is a property of Pool, not of the collector's own lease and + // scheduling goroutines this test exists to verify. + goleak.IgnoreAnyFunction("github.com/jackc/pgx/v5/pgxpool.(*Pool).backgroundHealthCheck"), + ) + }) pgURL := testSetup(t) @@ -196,6 +216,14 @@ func TestLeaseLifecycle(t *testing.T) { a.Eventually(func() bool { return hasMetric(survivorReg, "handover_databases_count") }, 10*time.Second, 500*time.Millisecond, "survivor never produced metrics after handover") + + // Stop both servers and wait for their goroutines to exit before the + // goleak check above runs, so a leaked lease-holder goroutine from a + // handover does not go unnoticed. + cancel1() + cancel2() + r.NoError(stop1.Wait()) + r.NoError(stop2.Wait()) } func hasMetric(registry *prometheus.Registry, name string) bool { diff --git a/internal/scanner/server_test.go b/internal/scanner/server_test.go index 88a93d6..cacc2a6 100644 --- a/internal/scanner/server_test.go +++ b/internal/scanner/server_test.go @@ -26,11 +26,22 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) func TestRefreshScanners(t *testing.T) { r := require.New(t) a := assert.New(t) + defer goleak.VerifyNone(t, + goleak.IgnoreCurrent(), + // nxadm/tail lazily starts a single, process-wide file-watcher + // goroutine pair the first time any file is tailed, and never + // stops them: they are reused by every Tail for the lifetime of + // the process. That is a property of the library, not a leak in + // Scanner's own Start/Stop. + goleak.IgnoreTopFunction("github.com/nxadm/tail/watch.(*InotifyTracker).run"), + goleak.IgnoreAnyFunction("github.com/fsnotify/fsnotify.(*Watcher).readEvents"), + ) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() @@ -169,6 +180,11 @@ func TestRefreshScanners(t *testing.T) { a.Contains([]string{"crdblog_cache", "crdblog_max", "pebble_sstable"}, *m.Name) } + // Stop the remaining scanner and wait for its goroutines to exit before + // the goleak check above runs, so a leaked goroutine from Start/Stop + // does not go unnoticed. + cancel() + r.NoError(stopperCtx.Wait()) } // TestRefreshScannersInvalidFormat verifies that a scan with an unsupported