PMM-14807 Enable the global connection pool by default

.github/workflows/lint.yml

@@ -47,7 +47,7 @@ jobs:
           filter_mode: added
           cache: false
           golangci_lint_flags: "--config=.golangci.yml"
-          golangci_lint_version: v2.12.2
+          golangci_lint_version: v2.13.1
 
       - name: Run license check
         run: |

.golangci.yml

@@ -6,8 +6,9 @@ linters:
     - gomodguard_v2
   disable:
     - exhaustruct
-    - gochecknoglobals # we need globals for better performance, we use them for mapping, and we don't mutate them, so it's safe.
-    - gomodguard # replaced by gomodguard_v2
+    - exhaustruct_v5    # too annoying, partial structs are fine
+    - gochecknoglobals  # we need globals for better performance, we use them for mapping, and we don't mutate them, so it's safe.
+    - gomodguard        # replaced by gomodguard_v2
     - lll
     - testpackage
     - varnamelen

REFERENCE.md

@@ -9,7 +9,7 @@
 | --mongodb.collstats-colls         | List of comma separared databases.collections to get $collStats                                                                                                               | --mongodb.collstats-colls=db1,db2.col2                           |
 | --mongodb.indexstats-colls        | List of comma separared databases.collections to get $indexStats                                                                                                              | --mongodb.indexstats-colls=db1.col1,db2.col2                     |
 | --[no-]mongodb.direct-connect     | Whether or not a direct connect should be made. Direct connections are not valid if multiple hosts are specified or an SRV URI is used                                        |                                                                  |
-| --[no-]mongodb.global-conn-pool   | Use global connection pool instead of creating new pool for each http request                                                                                                 |                                                                  |
+| --[no-]mongodb.global-conn-pool   | Use global connection pool instead of creating new pool for each http request. Enabled by default                                                                             | --no-mongodb.global-conn-pool                                    |
 | --mongodb.uri                     | MongoDB connection URI ($MONGODB_URI)                                                                                                                                         | --mongodb.uri=mongodb://user:pass@127.0.0.1:27017/admin?ssl=true |
 | --split-cluster                   | Whether to treat cluster members from the connection URI as separate targets                                                                                                  |
 | --web.listen-address              | Address to listen on for web interface and telemetry                                                                                                                          | --web.listen-address=":9216"                                     |

VERSION

@@ -1 +1 @@
-v0.53.0
+v0.54.0

exporter/exporter.go

@@ -18,6 +18,7 @@ package exporter
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"log/slog"
 	"net/http"
@@ -30,14 +31,19 @@ import (
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"github.com/prometheus/common/promslog"
 	"go.mongodb.org/mongo-driver/mongo"
+	"golang.org/x/sync/singleflight"
 
 	"github.com/percona/mongodb_exporter/exporter/dsn_fix"
 )
 
 // Exporter holds Exporter methods and attributes.
 type Exporter struct {
-	client                *mongo.Client
-	clientMu              sync.Mutex
+	client *mongo.Client
+	// clientMu guards the client pointer only. It is never held across a command, so
+	// concurrent scrapes of this target do not wait for each other.
+	clientMu sync.RWMutex
+	// clientGroup collapses concurrent attempts to build client into one connect.
+	clientGroup           singleflight.Group
 	logger                *slog.Logger
 	opts                  *Opts
 	lock                  *sync.Mutex
@@ -94,6 +100,12 @@ var (
 
 const (
 	defaultCacheSize = 1000
+
+	// defaultConnectTimeout bounds building the pooled client when Opts.ConnectTimeoutMS is
+	// unset. connect would otherwise hand the driver a server-selection timeout of 0, which
+	// it reads as no timeout at all, and an unanswering server would block a connect for the
+	// lifetime of the process.
+	defaultConnectTimeout = 30 * time.Second
 )
 
 // New connects to the database and returns a new Exporter instance.
@@ -107,19 +119,34 @@ func New(opts *Opts) *Exporter {
 		opts.Logger = promslog.New(promslogConfig)
 	}
 
-	ctx := context.Background()
-
 	exp := &Exporter{
 		logger:                opts.Logger,
 		opts:                  opts,
 		lock:                  &sync.Mutex{},
 		totalCollectionsCount: -1, // Not calculated yet. waiting the db connection.
 	}
 	// Try initial connect. Connection will be retried with every scrape.
+	// getClient bounds the connect itself, so no deadline is imposed here.
 	go func() {
-		_, err := exp.getClient(ctx)
+		ctx := context.Background()
+
+		client, err := exp.getClient(ctx)
 		if err != nil {
 			exp.logger.Error("Cannot connect to MongoDB", "error", err)
+
+			return
+		}
+
+		// With the global pool this client is the one every later scrape reuses. Otherwise
+		// nothing owns it, since each scrape builds its own, so leaving it connected would
+		// leak a topology, its monitoring goroutines and a connection.
+		if exp.opts.GlobalConnPool {
+			return
+		}
+
+		err = client.Disconnect(ctx)
+		if err != nil {
+			exp.logger.Error("Cannot disconnect client", "error", err)
 		}
 	}()
 
@@ -267,39 +294,82 @@ func (e *Exporter) makeRegistry(ctx context.Context, client *mongo.Client, topol
 	return registry
 }
 
+// getClient returns a client to scrape with. Everything below keeps to one rule: a scrape
+// must come back within its own budget, so that a struggling MongoDB is reported as
+// mongodb_up 0 rather than as a scrape Prometheus never gets an answer to.
 func (e *Exporter) getClient(ctx context.Context) (*mongo.Client, error) {
-	if e.opts.GlobalConnPool {
-		// Get global client. Maybe it must be initialized first.
-		// Initialization is retried with every scrape until it succeeds once.
-		e.clientMu.Lock()
-		defer e.clientMu.Unlock()
+	if !e.opts.GlobalConnPool {
+		// Create a new client for every scrape. The caller disconnects it.
+		return connect(ctx, e.opts)
+	}
 
-		// If client is already initialized, and Ping is successful -- return it.
-		if e.client != nil {
-			err := e.client.Ping(ctx, nil)
-			if err != nil {
-				return nil, fmt.Errorf("cannot connect to MongoDB: %w", err)
-			}
+	e.clientMu.RLock()
+	client := e.client
+	e.clientMu.RUnlock()
+
+	// Health-check outside the lock. Holding it across the Ping would make concurrent
+	// scrapes of this target queue behind each other, letting one slow scrape push the
+	// next past its budget.
+	if client != nil {
+		err := client.Ping(ctx, nil)
+		if err == nil {
+			return client, nil
+		}
 
-			return e.client, nil
+		// A disconnected client never recovers, so forget it and let the next scrape build a
+		// new one. Every other error is transient -- an unreachable server, a scrape that ran
+		// out of time -- and the driver reconnects the pool on its own; tearing it down would
+		// only add churn while MongoDB is already struggling.
+		if errors.Is(err, mongo.ErrClientDisconnected) {
+			e.logger.Warn("Dropping disconnected MongoDB client, reconnecting on next scrape")
+			e.clientMu.Lock()
+			if e.client == client {
+				e.client = nil
+			}
+			e.clientMu.Unlock()
 		}
 
-		client, err := connect(context.Background(), e.opts)
+		return nil, fmt.Errorf("cannot connect to MongoDB: %w", err)
+	}
+
+	// Build the client. Initialization is retried with every scrape until it succeeds once.
+	//
+	// The connect gets its own budget rather than the scrape's: a scrape shorter than one
+	// connect would cancel every attempt and leave the pool permanently empty, so every
+	// scrape would keep paying for a connect that can never finish. Concurrent scrapes
+	// collapse onto that one connect, and each gives up on its own deadline while it
+	// carries on in the background.
+	connectTimeout := time.Duration(e.opts.ConnectTimeoutMS) * time.Millisecond
+	if connectTimeout <= 0 {
+		connectTimeout = defaultConnectTimeout
+	}
+
+	built := e.clientGroup.DoChan("", func() (any, error) { //nolint:contextcheck
+		connectCtx, cancel := context.WithTimeout(context.Background(), connectTimeout)
+		defer cancel()
+
+		newClient, err := connect(connectCtx, e.opts)
 		if err != nil {
 			return nil, err
 		}
-		e.client = client
 
-		return client, nil
-	}
+		e.clientMu.Lock()
+		e.client = newClient
+		e.clientMu.Unlock()
 
-	// !e.opts.GlobalConnPool: create new client for every scrape.
-	client, err := connect(ctx, e.opts)
-	if err != nil {
-		return nil, err
-	}
+		return newClient, nil
+	})
 
-	return client, nil
+	select {
+	case res := <-built:
+		if res.Err != nil {
+			return nil, res.Err
+		}
+
+		return res.Val.(*mongo.Client), nil //nolint:forcetypeassert
+	case <-ctx.Done():
+		return nil, fmt.Errorf("cannot connect to MongoDB: %w", ctx.Err())
+	}
 }
 
 // Handler returns an http.Handler that serves metrics. Can be used instead of