Commit a6b3cc21 authored by Doug Barrett's avatar Doug Barrett
Browse files

fix(v2/postgres): Add Name(), connection lifetime config, and error prefixes

Address AGENTS.md review findings:
- Add Name field to Config with default "postgres" for multi-database
  identification in logs and errors
- Add ConnMaxLifetime and ConnMaxIdleTime config for PgBouncer compat
- Wrap Shutdown errors with the component name
- Use Start error prefix from c.name instead of hardcoded "postgres"
- Extract OTel attribute keys to package-level constants
parent 5868884e
Loading
Loading
Loading
Loading
+51 −11
Original line number Diff line number Diff line
@@ -4,6 +4,7 @@ import (
	"context"
	"database/sql"
	"fmt"
	"time"

	"github.com/jackc/pgx/v5"
	"github.com/jackc/pgx/v5/stdlib"
@@ -21,6 +22,10 @@ type Config struct {
	// connection strings (e.g. "host=localhost user=postgres dbname=mydb").
	DSN string

	// Name identifies this client in logs and errors. Defaults to "postgres".
	// Use distinct names when a service connects to multiple databases.
	Name string

	// MaxConns is the maximum number of open connections to the database.
	// Maps to [sql.DB.SetMaxOpenConns]. Zero means unlimited.
	MaxConns int
@@ -29,6 +34,17 @@ type Config struct {
	// pool. Maps to [sql.DB.SetMaxIdleConns]. Zero uses the default (2).
	MinConns int

	// ConnMaxLifetime is the maximum duration a connection may be reused.
	// Maps to [sql.DB.SetConnMaxLifetime]. Zero means connections are not
	// closed due to age. Set this when using PgBouncer or similar connection
	// poolers to ensure stale connections are recycled.
	ConnMaxLifetime time.Duration

	// ConnMaxIdleTime is the maximum duration a connection may sit idle
	// before being closed. Maps to [sql.DB.SetConnMaxIdleTime]. Zero means
	// connections are not closed due to idle time.
	ConnMaxIdleTime time.Duration

	// Tracer is used to create spans for queries. When nil, tracing is
	// disabled.
	Tracer *trace.Tracer
@@ -38,10 +54,13 @@ type Config struct {
// implements [app.Component]. Call [Start] to open the connection and
// [Shutdown] to close it.
type Client struct {
	name            string
	connCfg         *pgx.ConnConfig
	db              *sql.DB
	maxConns        int
	minConns        int
	connMaxLifetime time.Duration
	connMaxIdleTime time.Duration
}

// New returns a Client for the given DSN with no tracing. It is shorthand for
@@ -66,13 +85,26 @@ func NewWithConfig(cfg *Config) (*Client, error) {
		connCfg.Tracer = &queryTracer{tracer: cfg.Tracer}
	}

	name := cfg.Name
	if name == "" {
		name = "postgres"
	}

	return &Client{
		name:            name,
		connCfg:         connCfg,
		maxConns:        cfg.MaxConns,
		minConns:        cfg.MinConns,
		connMaxLifetime: cfg.ConnMaxLifetime,
		connMaxIdleTime: cfg.ConnMaxIdleTime,
	}, nil
}

// Name returns the component name for use in logs and error messages.
func (c *Client) Name() string {
	return c.name
}

// DB returns the underlying [*sql.DB]. It returns nil before [Start] has been
// called successfully.
func (c *Client) DB() *sql.DB {
@@ -80,8 +112,8 @@ func (c *Client) DB() *sql.DB {
}

// QueryTracer returns the [pgx.QueryTracer] configured on this client, or nil
// when no [Config.Tracer] was provided. The primary use-case is testing: the
// tracer can be exercised directly without a live database connection.
// when no [Config.Tracer] was provided. This is primarily useful for testing
// the tracing integration without a live database.
func (c *Client) QueryTracer() pgx.QueryTracer {
	return c.connCfg.Tracer
}
@@ -97,10 +129,16 @@ func (c *Client) Start(ctx context.Context) error {
	if c.minConns > 0 {
		db.SetMaxIdleConns(c.minConns)
	}
	if c.connMaxLifetime > 0 {
		db.SetConnMaxLifetime(c.connMaxLifetime)
	}
	if c.connMaxIdleTime > 0 {
		db.SetConnMaxIdleTime(c.connMaxIdleTime)
	}

	if err := db.PingContext(ctx); err != nil {
		db.Close()
		return fmt.Errorf("postgres: ping failed: %w", err)
		return fmt.Errorf("%s: ping failed: %w", c.name, err)
	}

	c.db = db
@@ -111,7 +149,9 @@ func (c *Client) Start(ctx context.Context) error {
// [app.Component] and should be called via [app.App.Shutdown].
func (c *Client) Shutdown(_ context.Context) error {
	if c.db != nil {
		return c.db.Close()
		if err := c.db.Close(); err != nil {
			return fmt.Errorf("%s: %w", c.name, err)
		}
	}
	return nil
}
+8 −2
Original line number Diff line number Diff line
@@ -8,6 +8,12 @@ import (
	"gitlab.com/gitlab-org/labkit/v2/trace"
)

// OpenTelemetry semantic convention attribute keys for database spans.
const (
	attrDBSystem    = "db.system"
	attrDBStatement = "db.statement"
)

// queryTracer implements [pgx.QueryTracer] and creates an OpenTelemetry span
// for every query, QueryRow, and Exec call when a Tracer is configured.
type queryTracer struct {
@@ -18,8 +24,8 @@ type queryTracer struct {
// and stores it in the returned context so TraceQueryEnd can retrieve it.
func (t *queryTracer) TraceQueryStart(ctx context.Context, _ *pgx.Conn, data pgx.TraceQueryStartData) context.Context {
	ctx, span := t.tracer.Start(ctx, spanName(data.SQL))
	span.SetAttribute("db.system", "postgresql")
	span.SetAttribute("db.statement", data.SQL)
	span.SetAttribute(attrDBSystem, "postgresql")
	span.SetAttribute(attrDBStatement, data.SQL)
	return ctx
}