From 4105dfd9f289c1d27658bfe4613b18eb7bccc767 Mon Sep 17 00:00:00 2001 From: idoko Date: Tue, 8 Oct 2024 01:20:47 +0100 Subject: [PATCH 1/7] add benchmark --- exporter/exporter_test.go | 50 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/exporter/exporter_test.go b/exporter/exporter_test.go index 8f59c48e..59ef7c17 100644 --- a/exporter/exporter_test.go +++ b/exporter/exporter_test.go @@ -26,6 +26,7 @@ import ( "strings" "sync" "testing" + "time" "github.com/prometheus/client_golang/prometheus/testutil" "github.com/sirupsen/logrus" @@ -257,3 +258,52 @@ func TestMongoUpMetric(t *testing.T) { }) } } +func BenchmarkExporterRegistry(b *testing.B) { + type testcase struct { + name string + URI string + } + + currentTC := testcase{name: "mongo-1-1", URI: fmt.Sprintf("mongodb://127.0.0.1:%s/admin", tu.GetenvDefault("TEST_MONGODB_S1_SECONDARY1_PORT", "27017"))} + + b.Run("cluster without PBM config", func(b *testing.B) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + exporterOpts := &Opts{ + Logger: logrus.New(), + URI: currentTC.URI, + ConnectTimeoutMS: 200, + DirectConnect: true, + GlobalConnPool: false, + CollectAll: true, + } + + client, err := connect(ctx, exporterOpts) + assert.NoError(b, err) + b.Cleanup(func() { + assert.NoError(b, client.Disconnect(ctx)) + }) + + mongoURI := "mongodb://127.0.0.1:17001/?connectTimeoutMS=1000&directConnection=true&serverSelectionTimeoutMS=1000" //nolint:gosec + + e := New(exporterOpts) + for i := 0; i < b.N; i++ { + gc := newPbmCollector(ctx, client, mongoURI, e.opts.Logger) + _ = e.makeRegistry(ctx, client, new(labelsGetterMock), *e.opts) + + filter := []string{ + "mongodb_pbm_cluster_backup_configured", + } + expected := strings.NewReader(` + # HELP mongodb_pbm_cluster_backup_configured PBM backups are configured for the cluster + # TYPE mongodb_pbm_cluster_backup_configured gauge + mongodb_pbm_cluster_backup_configured 1` + "\n") + err = testutil.CollectAndCompare(gc, expected, filter...) + assert.NotNil(b, err) // since PBM is not configured, we expect an error here. + + //res := r.Unregister(gc) + //assert.Equal(b, true, res) + } + b.ReportAllocs() + }) +} From e9e302bdc5a17326f794f8c61ca85df509c9c8e6 Mon Sep 17 00:00:00 2001 From: idoko Date: Tue, 8 Oct 2024 01:30:37 +0100 Subject: [PATCH 2/7] disable pbm collector if there's an error during init --- exporter/exporter.go | 10 ++++++++-- exporter/exporter_test.go | 16 ++-------------- exporter/pbm_collector.go | 15 +++++++++++++-- exporter/pbm_collector_test.go | 3 ++- 4 files changed, 25 insertions(+), 19 deletions(-) diff --git a/exporter/exporter.go b/exporter/exporter.go index 75a16606..0076df15 100644 --- a/exporter/exporter.go +++ b/exporter/exporter.go @@ -249,8 +249,14 @@ func (e *Exporter) makeRegistry(ctx context.Context, client *mongo.Client, topol } if e.opts.EnablePBMMetrics && requestOpts.EnablePBMMetrics { - pbmc := newPbmCollector(ctx, client, e.opts.URI, e.opts.Logger) - registry.MustRegister(pbmc) + pbmc, err := newPbmCollector(ctx, client, e.opts.URI, e.opts.Logger) + if err != nil { + e.logger.Errorf("Cannot create PBM collector: %v, collector will now be disabled.", err) + e.opts.EnablePBMMetrics = false + requestOpts.EnablePBMMetrics = false + } else { + registry.MustRegister(pbmc) + } } return registry diff --git a/exporter/exporter_test.go b/exporter/exporter_test.go index 59ef7c17..6a7b078e 100644 --- a/exporter/exporter_test.go +++ b/exporter/exporter_test.go @@ -288,21 +288,9 @@ func BenchmarkExporterRegistry(b *testing.B) { e := New(exporterOpts) for i := 0; i < b.N; i++ { - gc := newPbmCollector(ctx, client, mongoURI, e.opts.Logger) + _, err = newPbmCollector(ctx, client, mongoURI, e.opts.Logger) + assert.NotNil(b, err) _ = e.makeRegistry(ctx, client, new(labelsGetterMock), *e.opts) - - filter := []string{ - "mongodb_pbm_cluster_backup_configured", - } - expected := strings.NewReader(` - # HELP mongodb_pbm_cluster_backup_configured PBM backups are configured for the cluster - # TYPE mongodb_pbm_cluster_backup_configured gauge - mongodb_pbm_cluster_backup_configured 1` + "\n") - err = testutil.CollectAndCompare(gc, expected, filter...) - assert.NotNil(b, err) // since PBM is not configured, we expect an error here. - - //res := r.Unregister(gc) - //assert.Equal(b, true, res) } b.ReportAllocs() }) diff --git a/exporter/pbm_collector.go b/exporter/pbm_collector.go index fc613a2b..d797eaeb 100644 --- a/exporter/pbm_collector.go +++ b/exporter/pbm_collector.go @@ -53,17 +53,28 @@ func createPBMMetric(name, help string, value float64, labels map[string]string) return prometheus.MustNewConstMetric(d, prometheus.GaugeValue, value) } -func newPbmCollector(ctx context.Context, client *mongo.Client, mongoURI string, logger *logrus.Logger) *pbmCollector { +func newPbmCollector(ctx context.Context, client *mongo.Client, mongoURI string, logger *logrus.Logger) (*pbmCollector, error) { // we can't get details of other cluster from PBM if directConnection is set to true, // we re-write it if that option is set (e.g from PMM). if strings.Contains(mongoURI, "directConnection=true") { mongoURI = strings.ReplaceAll(mongoURI, "directConnection=true", "directConnection=false") } + pbmClient, err := sdk.NewClient(ctx, mongoURI) + if err != nil { + logger.Errorf("failed to initialize PBM client from uri %s: %s", mongoURI, err.Error()) + return nil, err + } + + _, err = pbmClient.GetConfig(ctx) + if err != nil { + logger.Errorf("failed to get PBM configuration during initialization: %s", err.Error()) + return nil, err + } return &pbmCollector{ ctx: ctx, mongoURI: mongoURI, base: newBaseCollector(client, logger.WithFields(logrus.Fields{"collector": "pbm"})), - } + }, nil } func (p *pbmCollector) Describe(ch chan<- *prometheus.Desc) { diff --git a/exporter/pbm_collector_test.go b/exporter/pbm_collector_test.go index 5a03599b..7008bb28 100644 --- a/exporter/pbm_collector_test.go +++ b/exporter/pbm_collector_test.go @@ -41,7 +41,8 @@ func TestPBMCollector(t *testing.T) { client := tu.TestClient(ctx, port, t) mongoURI := "mongodb://admin:admin@127.0.0.1:17006/?connectTimeoutMS=1000&directConnection=true&serverSelectionTimeoutMS=1000" //nolint:gosec - c := newPbmCollector(ctx, client, mongoURI, logrus.New()) + c, err := newPbmCollector(ctx, client, mongoURI, logrus.New()) + require.NoError(t, err) t.Run("pbm configured metric", func(t *testing.T) { filter := []string{ From 8e1a04a201e3fe7e695ea25ad2762be3e6197f37 Mon Sep 17 00:00:00 2001 From: idoko Date: Tue, 8 Oct 2024 01:36:09 +0100 Subject: [PATCH 3/7] drop benchmark code --- exporter/exporter_test.go | 45 +++------------------------------------ 1 file changed, 3 insertions(+), 42 deletions(-) diff --git a/exporter/exporter_test.go b/exporter/exporter_test.go index 6a7b078e..0b2d6844 100644 --- a/exporter/exporter_test.go +++ b/exporter/exporter_test.go @@ -18,6 +18,9 @@ package exporter import ( "context" "fmt" + "github.com/prometheus/client_golang/prometheus/testutil" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" "io" "net" "net/http" @@ -26,11 +29,6 @@ import ( "strings" "sync" "testing" - "time" - - "github.com/prometheus/client_golang/prometheus/testutil" - "github.com/sirupsen/logrus" - "github.com/stretchr/testify/assert" "github.com/percona/mongodb_exporter/internal/tu" ) @@ -258,40 +256,3 @@ func TestMongoUpMetric(t *testing.T) { }) } } -func BenchmarkExporterRegistry(b *testing.B) { - type testcase struct { - name string - URI string - } - - currentTC := testcase{name: "mongo-1-1", URI: fmt.Sprintf("mongodb://127.0.0.1:%s/admin", tu.GetenvDefault("TEST_MONGODB_S1_SECONDARY1_PORT", "27017"))} - - b.Run("cluster without PBM config", func(b *testing.B) { - ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) - defer cancel() - exporterOpts := &Opts{ - Logger: logrus.New(), - URI: currentTC.URI, - ConnectTimeoutMS: 200, - DirectConnect: true, - GlobalConnPool: false, - CollectAll: true, - } - - client, err := connect(ctx, exporterOpts) - assert.NoError(b, err) - b.Cleanup(func() { - assert.NoError(b, client.Disconnect(ctx)) - }) - - mongoURI := "mongodb://127.0.0.1:17001/?connectTimeoutMS=1000&directConnection=true&serverSelectionTimeoutMS=1000" //nolint:gosec - - e := New(exporterOpts) - for i := 0; i < b.N; i++ { - _, err = newPbmCollector(ctx, client, mongoURI, e.opts.Logger) - assert.NotNil(b, err) - _ = e.makeRegistry(ctx, client, new(labelsGetterMock), *e.opts) - } - b.ReportAllocs() - }) -} From 8b9e5f7fa08736e49ce245cf558850a25edb18c8 Mon Sep 17 00:00:00 2001 From: idoko Date: Tue, 8 Oct 2024 08:55:36 +0100 Subject: [PATCH 4/7] close pbm client --- exporter/pbm_collector.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/exporter/pbm_collector.go b/exporter/pbm_collector.go index d797eaeb..50d35234 100644 --- a/exporter/pbm_collector.go +++ b/exporter/pbm_collector.go @@ -65,6 +65,12 @@ func newPbmCollector(ctx context.Context, client *mongo.Client, mongoURI string, return nil, err } + defer func() { + err := pbmClient.Close(ctx) + if err != nil { + logger.Errorf("failed to close PBM client: %v", err) + } + }() _, err = pbmClient.GetConfig(ctx) if err != nil { logger.Errorf("failed to get PBM configuration during initialization: %s", err.Error()) @@ -97,6 +103,12 @@ func (p *pbmCollector) collect(ch chan<- prometheus.Metric) { logger.Errorf("failed to create PBM client from uri %s: %s", p.mongoURI, err.Error()) return } + defer func() { + err := pbmClient.Close(p.ctx) + if err != nil { + logger.Errorf("failed to close PBM client: %v", err) + } + }() pbmConfig, err := pbmClient.GetConfig(p.ctx) if err != nil { From 3ebb401e552d6117317ab1e37f56cb856860473d Mon Sep 17 00:00:00 2001 From: idoko Date: Wed, 9 Oct 2024 21:13:35 +0100 Subject: [PATCH 5/7] format imports --- exporter/exporter_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/exporter/exporter_test.go b/exporter/exporter_test.go index 0b2d6844..8f59c48e 100644 --- a/exporter/exporter_test.go +++ b/exporter/exporter_test.go @@ -18,9 +18,6 @@ package exporter import ( "context" "fmt" - "github.com/prometheus/client_golang/prometheus/testutil" - "github.com/sirupsen/logrus" - "github.com/stretchr/testify/assert" "io" "net" "net/http" @@ -30,6 +27,10 @@ import ( "sync" "testing" + "github.com/prometheus/client_golang/prometheus/testutil" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/percona/mongodb_exporter/internal/tu" ) From 7af416dbdeb40e35d8333f7454a26427a36c3f28 Mon Sep 17 00:00:00 2001 From: idoko Date: Thu, 10 Oct 2024 10:25:43 +0100 Subject: [PATCH 6/7] drop error log --- exporter/exporter.go | 10 ++-------- exporter/pbm_collector.go | 23 +++-------------------- exporter/pbm_collector_test.go | 3 +-- 3 files changed, 6 insertions(+), 30 deletions(-) diff --git a/exporter/exporter.go b/exporter/exporter.go index 0076df15..75a16606 100644 --- a/exporter/exporter.go +++ b/exporter/exporter.go @@ -249,14 +249,8 @@ func (e *Exporter) makeRegistry(ctx context.Context, client *mongo.Client, topol } if e.opts.EnablePBMMetrics && requestOpts.EnablePBMMetrics { - pbmc, err := newPbmCollector(ctx, client, e.opts.URI, e.opts.Logger) - if err != nil { - e.logger.Errorf("Cannot create PBM collector: %v, collector will now be disabled.", err) - e.opts.EnablePBMMetrics = false - requestOpts.EnablePBMMetrics = false - } else { - registry.MustRegister(pbmc) - } + pbmc := newPbmCollector(ctx, client, e.opts.URI, e.opts.Logger) + registry.MustRegister(pbmc) } return registry diff --git a/exporter/pbm_collector.go b/exporter/pbm_collector.go index 50d35234..8ec975fc 100644 --- a/exporter/pbm_collector.go +++ b/exporter/pbm_collector.go @@ -53,34 +53,17 @@ func createPBMMetric(name, help string, value float64, labels map[string]string) return prometheus.MustNewConstMetric(d, prometheus.GaugeValue, value) } -func newPbmCollector(ctx context.Context, client *mongo.Client, mongoURI string, logger *logrus.Logger) (*pbmCollector, error) { +func newPbmCollector(ctx context.Context, client *mongo.Client, mongoURI string, logger *logrus.Logger) *pbmCollector { // we can't get details of other cluster from PBM if directConnection is set to true, // we re-write it if that option is set (e.g from PMM). if strings.Contains(mongoURI, "directConnection=true") { mongoURI = strings.ReplaceAll(mongoURI, "directConnection=true", "directConnection=false") } - pbmClient, err := sdk.NewClient(ctx, mongoURI) - if err != nil { - logger.Errorf("failed to initialize PBM client from uri %s: %s", mongoURI, err.Error()) - return nil, err - } - - defer func() { - err := pbmClient.Close(ctx) - if err != nil { - logger.Errorf("failed to close PBM client: %v", err) - } - }() - _, err = pbmClient.GetConfig(ctx) - if err != nil { - logger.Errorf("failed to get PBM configuration during initialization: %s", err.Error()) - return nil, err - } return &pbmCollector{ ctx: ctx, mongoURI: mongoURI, base: newBaseCollector(client, logger.WithFields(logrus.Fields{"collector": "pbm"})), - }, nil + } } func (p *pbmCollector) Describe(ch chan<- *prometheus.Desc) { @@ -112,7 +95,7 @@ func (p *pbmCollector) collect(ch chan<- prometheus.Metric) { pbmConfig, err := pbmClient.GetConfig(p.ctx) if err != nil { - logger.Errorf("failed to get PBM configuration: %s", err.Error()) + logger.Warnf("failed to get PBM configuration: %s", err.Error()) } if pbmConfig != nil { diff --git a/exporter/pbm_collector_test.go b/exporter/pbm_collector_test.go index 7008bb28..5a03599b 100644 --- a/exporter/pbm_collector_test.go +++ b/exporter/pbm_collector_test.go @@ -41,8 +41,7 @@ func TestPBMCollector(t *testing.T) { client := tu.TestClient(ctx, port, t) mongoURI := "mongodb://admin:admin@127.0.0.1:17006/?connectTimeoutMS=1000&directConnection=true&serverSelectionTimeoutMS=1000" //nolint:gosec - c, err := newPbmCollector(ctx, client, mongoURI, logrus.New()) - require.NoError(t, err) + c := newPbmCollector(ctx, client, mongoURI, logrus.New()) t.Run("pbm configured metric", func(t *testing.T) { filter := []string{ From 1284aceae1ec1ae50d0304ab559f5a25d1500023 Mon Sep 17 00:00:00 2001 From: idoko Date: Thu, 10 Oct 2024 10:33:03 +0100 Subject: [PATCH 7/7] drop log level to info --- exporter/pbm_collector.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporter/pbm_collector.go b/exporter/pbm_collector.go index 8ec975fc..a44a1b61 100644 --- a/exporter/pbm_collector.go +++ b/exporter/pbm_collector.go @@ -95,7 +95,7 @@ func (p *pbmCollector) collect(ch chan<- prometheus.Metric) { pbmConfig, err := pbmClient.GetConfig(p.ctx) if err != nil { - logger.Warnf("failed to get PBM configuration: %s", err.Error()) + logger.Infof("failed to get PBM configuration: %s", err.Error()) } if pbmConfig != nil {