From cac4625a5640c250ec4ff5dc54acbf3c8a1b8ba5 Mon Sep 17 00:00:00 2001 From: Ian Oberst Date: Thu, 23 Jul 2026 07:48:48 -0700 Subject: [PATCH 1/5] Add PostgreSQL module foundations Introduce explicit monitor database typing and PostgreSQL configuration while preserving MySQL defaults. Extract shared credential source construction without changing MySQL-specific reload behavior. Co-authored-by: Codex Ai-assisted: true Co-authored-by: Goose --- blip.go | 2 +- config.go | 106 +++++++++++++++--- config_postgres.go | 187 ++++++++++++++++++++++++++++++++ config_postgres_test.go | 179 ++++++++++++++++++++++++++++++ credentials/credentials.go | 112 +++++++++++++++++++ credentials/credentials_test.go | 115 ++++++++++++++++++++ dbconn/credentials_test.go | 55 ++++++++++ dbconn/factory.go | 86 +-------------- dbconn/password_secret_test.go | 10 +- dbconn/reload_password.go | 5 +- 10 files changed, 752 insertions(+), 105 deletions(-) create mode 100644 config_postgres.go create mode 100644 config_postgres_test.go create mode 100644 credentials/credentials.go create mode 100644 credentials/credentials_test.go create mode 100644 dbconn/credentials_test.go diff --git a/blip.go b/blip.go index eca1686..5db5fff 100644 --- a/blip.go +++ b/blip.go @@ -173,7 +173,7 @@ type Plugins struct { // ModifyDB modifies the *sql.DB connection pool. Use with caution. ModifyDB func(*sql.DB, string) - // ParsePasswordSecret maps an AWS Secrets Manager payload to MySQL credentials. + // ParsePasswordSecret maps an AWS Secrets Manager payload to database credentials. // If nil, Blip uses DefaultPasswordSecretParser. ParsePasswordSecret PasswordSecretParser diff --git a/config.go b/config.go index 2a5b851..1ac52de 100644 --- a/config.go +++ b/config.go @@ -399,6 +399,9 @@ func (c *ConfigMonitorLoader) ApplyDefaults(b Config) { type ConfigMonitor struct { MonitorId string `yaml:"id"` + // DatabaseType selects the database-specific connection and metric module. + // Empty values retain Blip's historical MySQL behavior. + DatabaseType DatabaseType `yaml:"database-type,omitempty"` // ConfigMySQL: Socket string `yaml:"socket,omitempty"` @@ -419,6 +422,7 @@ type ConfigMonitor struct { Heartbeat ConfigHeartbeat `yaml:"heartbeat,omitempty"` Plans ConfigPlans `yaml:"plans,omitempty"` Plan string `yaml:"plan,omitempty"` + Postgres ConfigPostgres `yaml:"postgres,omitempty"` Sinks ConfigSinks `yaml:"sinks,omitempty"` TLS ConfigTLS `yaml:"tls,omitempty"` @@ -430,6 +434,13 @@ const ( DEFAULT_MONITOR_TIMEOUT_CONNECT = "10s" ) +type DatabaseType string + +const ( + DatabaseTypeMySQL DatabaseType = "mysql" + DatabaseTypePostgres DatabaseType = "postgres" +) + func DefaultConfigMonitor() ConfigMonitor { return ConfigMonitor{ Username: DEFAULT_MONITOR_USERNAME, @@ -447,7 +458,35 @@ func DefaultConfigMonitor() ConfigMonitor { } } +// EffectiveDatabaseType returns the configured database type. An omitted +// value retains Blip's historical MySQL behavior. Environment interpolation is +// resolved here because monitor defaults are applied before the normal +// interpolation pass. +func (c ConfigMonitor) EffectiveDatabaseType() DatabaseType { + databaseType := DatabaseType(interpolateEnv(string(c.DatabaseType))) + if databaseType == "" { + return DatabaseTypeMySQL + } + return databaseType +} + func (c ConfigMonitor) Validate() error { + switch c.EffectiveDatabaseType() { + case DatabaseTypeMySQL: + if c.Postgres.Set() { + return fmt.Errorf("config.monitor.postgres requires database-type %q", DatabaseTypePostgres) + } + case DatabaseTypePostgres: + if c.Socket != "" { + return fmt.Errorf("config.monitor.socket is only supported for database-type %q", DatabaseTypeMySQL) + } + if c.MyCnf != "" { + return fmt.Errorf("config.monitor.mycnf is only supported for database-type %q", DatabaseTypeMySQL) + } + return c.Postgres.Validate() + default: + return fmt.Errorf("config.monitor.database-type: invalid database type %q", c.DatabaseType) + } return nil } @@ -464,23 +503,32 @@ func (c ConfigMonitor) redacted(seen map[*ConfigMonitor]*ConfigMonitor) ConfigMo } func (c *ConfigMonitor) ApplyDefaults(b Config) { - if c.Socket == "" { - c.Socket = b.MySQL.Socket - } - if c.Hostname == "" { - c.Hostname = b.MySQL.Hostname - } - if c.MyCnf == "" && b.MySQL.MyCnf != "" { - c.MyCnf = b.MySQL.MyCnf - } - if c.Username == "" && b.MySQL.Username != "" { - c.Username = b.MySQL.Username - } - if c.Password == "" && b.MySQL.Password != "" { - c.Password = b.MySQL.Password - } - if c.TimeoutConnect == "" && b.MySQL.TimeoutConnect != "" { - c.TimeoutConnect = b.MySQL.TimeoutConnect + if c.EffectiveDatabaseType() == DatabaseTypeMySQL { + if c.Socket == "" { + c.Socket = b.MySQL.Socket + } + if c.Hostname == "" { + c.Hostname = b.MySQL.Hostname + } + if c.MyCnf == "" && b.MySQL.MyCnf != "" { + c.MyCnf = b.MySQL.MyCnf + } + if c.Username == "" && b.MySQL.Username != "" { + c.Username = b.MySQL.Username + } + if c.Password == "" && b.MySQL.Password != "" { + c.Password = b.MySQL.Password + } + if c.TimeoutConnect == "" && b.MySQL.TimeoutConnect != "" { + c.TimeoutConnect = b.MySQL.TimeoutConnect + } + } else { + if c.Username == "" { + c.Username = DEFAULT_MONITOR_USERNAME + } + if c.TimeoutConnect == "" { + c.TimeoutConnect = DEFAULT_MONITOR_TIMEOUT_CONNECT + } } if len(b.Tags) > 0 { if c.Tags == nil { @@ -501,12 +549,18 @@ func (c *ConfigMonitor) ApplyDefaults(b Config) { c.HA.ApplyDefaults(b) c.Heartbeat.ApplyDefaults(b) c.Plans.ApplyDefaults(b) + if c.EffectiveDatabaseType() == DatabaseTypePostgres { + postgresDefaults := DefaultConfigPostgres() + postgresDefaults.ConnectTimeout = c.TimeoutConnect + c.Postgres.ApplyDefaults(postgresDefaults) + } c.Sinks.ApplyDefaults(b) c.TLS.ApplyDefaults(b) } func (c *ConfigMonitor) InterpolateEnvVars() { c.MonitorId = interpolateEnv(c.MonitorId) + c.DatabaseType = DatabaseType(interpolateEnv(string(c.DatabaseType))) c.MyCnf = interpolateEnv(c.MyCnf) c.Socket = interpolateEnv(c.Socket) c.Hostname = interpolateEnv(c.Hostname) @@ -526,6 +580,7 @@ func (c *ConfigMonitor) InterpolateEnvVars() { c.Heartbeat.InterpolateEnvVars() c.Plans.InterpolateEnvVars() c.Plan = interpolateEnv(c.Plan) + c.Postgres.InterpolateEnvVars() c.Sinks.InterpolateEnvVars() c.TLS.InterpolateEnvVars() } @@ -551,6 +606,7 @@ func (c *ConfigMonitor) InterpolateMonitor() { c.Heartbeat.InterpolateMonitor(c) c.Plans.InterpolateMonitor(c) c.Plan = c.interpolateMon(c.Plan) + c.Postgres.InterpolateMonitor(c) c.Sinks.InterpolateMonitor(c) c.TLS.InterpolateMonitor(c) } @@ -586,6 +642,8 @@ func (c *ConfigMonitor) fieldValue(f string) string { switch strings.ToLower(f) { case "monitorid", "monitor-id", "id": return c.MonitorId + case "database-type": + return string(c.DatabaseType) case "mycnf": return c.MyCnf case "socket": @@ -600,6 +658,20 @@ func (c *ConfigMonitor) fieldValue(f string) string { return c.PasswordFile case "timeout-connect": return c.TimeoutConnect + case "postgres.database": + return c.Postgres.Database + case "postgres.application-name": + return c.Postgres.ApplicationName + case "postgres.ssl-mode": + return c.Postgres.SSLMode + case "postgres.connect-timeout": + return c.Postgres.ConnectTimeout + case "postgres.statement-timeout": + return c.Postgres.StatementTimeout + case "postgres.lock-timeout": + return c.Postgres.LockTimeout + case "postgres.dial-address": + return c.Postgres.DialAddress default: return "" } diff --git a/config_postgres.go b/config_postgres.go new file mode 100644 index 0000000..fd13a06 --- /dev/null +++ b/config_postgres.go @@ -0,0 +1,187 @@ +// Copyright 2026 Block, Inc. + +package blip + +import ( + "fmt" + "strings" + "time" +) + +const ( + DEFAULT_POSTGRES_DATABASE = "postgres" + DEFAULT_POSTGRES_APPLICATION_NAME = "pgblip" + DEFAULT_POSTGRES_MAX_OPEN_CONNECTIONS = 4 + DEFAULT_POSTGRES_MAX_IDLE_CONNECTIONS = 2 + DEFAULT_POSTGRES_MAX_CONNECTION_IDLE_TIME = "30s" + DEFAULT_POSTGRES_MAX_CONNECTION_LIFETIME = "0" +) + +// ConfigPostgres configures the PostgreSQL database/sql pool owned by one +// monitor. Credentials and TLS certificate files remain in the existing +// monitor-level fields so all Blip credential sources can be shared by +// database-specific connection factories. +type ConfigPostgres struct { + Database string `yaml:"database,omitempty"` + ApplicationName string `yaml:"application-name,omitempty"` + SSLMode string `yaml:"ssl-mode,omitempty"` + MaxOpenConnections *int `yaml:"max-open-connections,omitempty"` + MaxIdleConnections *int `yaml:"max-idle-connections,omitempty"` + MaxConnectionIdleTime string `yaml:"max-connection-idle-time,omitempty"` + MaxConnectionLifetime string `yaml:"max-connection-lifetime,omitempty"` + ConnectTimeout string `yaml:"connect-timeout,omitempty"` + StatementTimeout string `yaml:"statement-timeout,omitempty"` + LockTimeout string `yaml:"lock-timeout,omitempty"` + DialAddress string `yaml:"dial-address,omitempty"` +} + +func DefaultConfigPostgres() ConfigPostgres { + return ConfigPostgres{ + Database: DEFAULT_POSTGRES_DATABASE, + ApplicationName: DEFAULT_POSTGRES_APPLICATION_NAME, + MaxOpenConnections: postgresInt(DEFAULT_POSTGRES_MAX_OPEN_CONNECTIONS), + MaxIdleConnections: postgresInt(DEFAULT_POSTGRES_MAX_IDLE_CONNECTIONS), + MaxConnectionIdleTime: DEFAULT_POSTGRES_MAX_CONNECTION_IDLE_TIME, + MaxConnectionLifetime: DEFAULT_POSTGRES_MAX_CONNECTION_LIFETIME, + ConnectTimeout: DEFAULT_MONITOR_TIMEOUT_CONNECT, + } +} + +// Set reports whether a monitor explicitly contains PostgreSQL configuration. +func (c ConfigPostgres) Set() bool { + return c.Database != "" || + c.ApplicationName != "" || + c.SSLMode != "" || + c.MaxOpenConnections != nil || + c.MaxIdleConnections != nil || + c.MaxConnectionIdleTime != "" || + c.MaxConnectionLifetime != "" || + c.ConnectTimeout != "" || + c.StatementTimeout != "" || + c.LockTimeout != "" || + c.DialAddress != "" +} + +func (c *ConfigPostgres) ApplyDefaults(defaults ConfigPostgres) { + if c.Database == "" { + c.Database = defaults.Database + } + if c.ApplicationName == "" { + c.ApplicationName = defaults.ApplicationName + } + if c.SSLMode == "" { + c.SSLMode = defaults.SSLMode + } + c.MaxOpenConnections = setPostgresInt(c.MaxOpenConnections, defaults.MaxOpenConnections) + c.MaxIdleConnections = setPostgresInt(c.MaxIdleConnections, defaults.MaxIdleConnections) + if c.MaxConnectionIdleTime == "" { + c.MaxConnectionIdleTime = defaults.MaxConnectionIdleTime + } + if c.MaxConnectionLifetime == "" { + c.MaxConnectionLifetime = defaults.MaxConnectionLifetime + } + if c.ConnectTimeout == "" { + c.ConnectTimeout = defaults.ConnectTimeout + } + if c.StatementTimeout == "" { + c.StatementTimeout = defaults.StatementTimeout + } + if c.LockTimeout == "" { + c.LockTimeout = defaults.LockTimeout + } + if c.DialAddress == "" { + c.DialAddress = defaults.DialAddress + } +} + +func (c ConfigPostgres) Validate() error { + validSSLModes := map[string]bool{ + "": true, + "disable": true, + "allow": true, + "prefer": true, + "require": true, + "verify-ca": true, + "verify-full": true, + } + if !validSSLModes[strings.ToLower(c.SSLMode)] { + return fmt.Errorf("config.postgres.ssl-mode: invalid PostgreSQL SSL mode %q", c.SSLMode) + } + if c.MaxOpenConnections != nil && *c.MaxOpenConnections < 0 { + return fmt.Errorf("config.postgres.max-open-connections: must be greater than or equal to zero") + } + if c.MaxIdleConnections != nil && *c.MaxIdleConnections < 0 { + return fmt.Errorf("config.postgres.max-idle-connections: must be greater than or equal to zero") + } + if c.MaxOpenConnections != nil && c.MaxIdleConnections != nil && + *c.MaxOpenConnections > 0 && *c.MaxIdleConnections > *c.MaxOpenConnections { + return fmt.Errorf("config.postgres.max-idle-connections: cannot exceed max-open-connections") + } + if err := validatePostgresDuration("connect-timeout", c.ConnectTimeout, false); err != nil { + return err + } + if err := validatePostgresDuration("max-connection-idle-time", c.MaxConnectionIdleTime, true); err != nil { + return err + } + if err := validatePostgresDuration("max-connection-lifetime", c.MaxConnectionLifetime, true); err != nil { + return err + } + if err := validatePostgresDuration("statement-timeout", c.StatementTimeout, true); err != nil { + return err + } + return validatePostgresDuration("lock-timeout", c.LockTimeout, true) +} + +func postgresInt(value int) *int { + return &value +} + +func setPostgresInt(value, defaultValue *int) *int { + if value != nil || defaultValue == nil { + return value + } + copy := *defaultValue + return © +} + +func validatePostgresDuration(name, value string, allowZero bool) error { + if value == "" { + return nil + } + duration, err := time.ParseDuration(value) + if err != nil { + return fmt.Errorf("config.postgres.%s: invalid duration %q: %w", name, value, err) + } + if duration < 0 || (!allowZero && duration == 0) { + constraint := "greater than zero" + if allowZero { + constraint = "greater than or equal to zero" + } + return fmt.Errorf("config.postgres.%s: must be %s", name, constraint) + } + return nil +} + +func (c *ConfigPostgres) InterpolateEnvVars() { + c.Database = interpolateEnv(c.Database) + c.ApplicationName = interpolateEnv(c.ApplicationName) + c.SSLMode = interpolateEnv(c.SSLMode) + c.MaxConnectionIdleTime = interpolateEnv(c.MaxConnectionIdleTime) + c.MaxConnectionLifetime = interpolateEnv(c.MaxConnectionLifetime) + c.ConnectTimeout = interpolateEnv(c.ConnectTimeout) + c.StatementTimeout = interpolateEnv(c.StatementTimeout) + c.LockTimeout = interpolateEnv(c.LockTimeout) + c.DialAddress = interpolateEnv(c.DialAddress) +} + +func (c *ConfigPostgres) InterpolateMonitor(m *ConfigMonitor) { + c.Database = m.interpolateMon(c.Database) + c.ApplicationName = m.interpolateMon(c.ApplicationName) + c.SSLMode = m.interpolateMon(c.SSLMode) + c.MaxConnectionIdleTime = m.interpolateMon(c.MaxConnectionIdleTime) + c.MaxConnectionLifetime = m.interpolateMon(c.MaxConnectionLifetime) + c.ConnectTimeout = m.interpolateMon(c.ConnectTimeout) + c.StatementTimeout = m.interpolateMon(c.StatementTimeout) + c.LockTimeout = m.interpolateMon(c.LockTimeout) + c.DialAddress = m.interpolateMon(c.DialAddress) +} diff --git a/config_postgres_test.go b/config_postgres_test.go new file mode 100644 index 0000000..8c3ae43 --- /dev/null +++ b/config_postgres_test.go @@ -0,0 +1,179 @@ +// Copyright 2026 Block, Inc. + +package blip_test + +import ( + "strings" + "testing" + + "github.com/cashapp/blip/v2" +) + +func TestConfigMonitorDatabaseTypeDefaultsToMySQLWithoutMutation(t *testing.T) { + monitor := blip.ConfigMonitor{} + monitor.ApplyDefaults(blip.DefaultConfig()) + + if monitor.DatabaseType != "" { + t.Fatalf("omitted database type mutated to %q", monitor.DatabaseType) + } + if monitor.EffectiveDatabaseType() != blip.DatabaseTypeMySQL { + t.Fatalf("effective database type = %q, expected mysql", monitor.EffectiveDatabaseType()) + } + if monitor.Postgres.Set() { + t.Fatalf("PostgreSQL defaults added to MySQL monitor: %+v", monitor.Postgres) + } +} + +func TestConfigMonitorPostgresDefaultsAndInterpolation(t *testing.T) { + t.Setenv("BLIP_TEST_DATABASE_TYPE", "postgres") + t.Setenv("BLIP_TEST_POSTGRES_DATABASE", "metrics_database") + t.Setenv("BLIP_TEST_POSTGRES_DIAL_ADDRESS", "127.0.0.1:35432") + + monitor := blip.ConfigMonitor{ + MonitorId: "postgres-monitor", + DatabaseType: "${BLIP_TEST_DATABASE_TYPE}", + TimeoutConnect: "7s", + Postgres: blip.ConfigPostgres{ + Database: "${BLIP_TEST_POSTGRES_DATABASE}", + ApplicationName: "%{monitor.id}", + DialAddress: "${BLIP_TEST_POSTGRES_DIAL_ADDRESS}", + }, + } + monitor.ApplyDefaults(blip.DefaultConfig()) + monitor.InterpolateEnvVars() + monitor.InterpolateMonitor() + + if err := monitor.Validate(); err != nil { + t.Fatal(err) + } + if monitor.DatabaseType != blip.DatabaseTypePostgres { + t.Fatalf("database type = %q, expected postgres", monitor.DatabaseType) + } + if monitor.Postgres.Database != "metrics_database" { + t.Fatalf("database = %q, expected metrics_database", monitor.Postgres.Database) + } + if monitor.Postgres.ApplicationName != monitor.MonitorId { + t.Fatalf("application name = %q, expected monitor ID %q", monitor.Postgres.ApplicationName, monitor.MonitorId) + } + if monitor.Postgres.DialAddress != "127.0.0.1:35432" { + t.Fatalf("dial address = %q, expected interpolated address", monitor.Postgres.DialAddress) + } + if monitor.Postgres.ConnectTimeout != "7s" { + t.Fatalf("connect timeout = %q, expected inherited monitor timeout", monitor.Postgres.ConnectTimeout) + } + if monitor.Postgres.MaxOpenConnections == nil || *monitor.Postgres.MaxOpenConnections != blip.DEFAULT_POSTGRES_MAX_OPEN_CONNECTIONS { + t.Fatalf("max open connections not defaulted: %+v", monitor.Postgres.MaxOpenConnections) + } + if monitor.Postgres.MaxIdleConnections == nil || *monitor.Postgres.MaxIdleConnections != blip.DEFAULT_POSTGRES_MAX_IDLE_CONNECTIONS { + t.Fatalf("max idle connections not defaulted: %+v", monitor.Postgres.MaxIdleConnections) + } +} + +func TestConfigMonitorDatabaseTypeValidation(t *testing.T) { + tests := []struct { + name string + monitor blip.ConfigMonitor + wantError string + }{ + { + name: "unknown database type", + monitor: blip.ConfigMonitor{DatabaseType: "oracle"}, + wantError: "invalid database type", + }, + { + name: "PostgreSQL config on implicit MySQL monitor", + monitor: blip.ConfigMonitor{ + Postgres: blip.ConfigPostgres{Database: "postgres"}, + }, + wantError: "requires database-type", + }, + { + name: "my.cnf on PostgreSQL monitor", + monitor: blip.ConfigMonitor{ + DatabaseType: blip.DatabaseTypePostgres, + MyCnf: "/etc/blip/my.cnf", + }, + wantError: "mycnf is only supported", + }, + { + name: "socket on PostgreSQL monitor", + monitor: blip.ConfigMonitor{ + DatabaseType: blip.DatabaseTypePostgres, + Socket: "/tmp/.s.PGSQL.5432", + }, + wantError: "socket is only supported", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.monitor.Validate() + if err == nil || !strings.Contains(err.Error(), tt.wantError) { + t.Fatalf("got error %v, expected it to contain %q", err, tt.wantError) + } + }) + } +} + +func TestConfigPostgresAllowsExplicitUnlimitedPoolSettings(t *testing.T) { + zero := 0 + config := blip.ConfigPostgres{ + Database: "postgres", + MaxOpenConnections: &zero, + MaxIdleConnections: &zero, + } + config.ApplyDefaults(blip.DefaultConfigPostgres()) + + if *config.MaxOpenConnections != 0 || *config.MaxIdleConnections != 0 { + t.Fatalf("explicit zero pool settings were overwritten: %+v", config) + } +} + +func TestConfigPostgresValidation(t *testing.T) { + minusOne := -1 + one := 1 + two := 2 + tests := []struct { + name string + config blip.ConfigPostgres + wantError string + }{ + { + name: "invalid SSL mode", + config: blip.ConfigPostgres{SSLMode: "invalid"}, + wantError: "ssl-mode", + }, + { + name: "negative max open", + config: blip.ConfigPostgres{MaxOpenConnections: &minusOne}, + wantError: "max-open-connections", + }, + { + name: "idle exceeds open", + config: blip.ConfigPostgres{ + MaxOpenConnections: &one, + MaxIdleConnections: &two, + }, + wantError: "cannot exceed", + }, + { + name: "zero connect timeout", + config: blip.ConfigPostgres{ConnectTimeout: "0"}, + wantError: "connect-timeout", + }, + { + name: "invalid lifetime", + config: blip.ConfigPostgres{MaxConnectionLifetime: "tomorrow"}, + wantError: "max-connection-lifetime", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.config.Validate() + if err == nil || !strings.Contains(err.Error(), tt.wantError) { + t.Fatalf("got error %v, expected it to contain %q", err, tt.wantError) + } + }) + } +} diff --git a/credentials/credentials.go b/credentials/credentials.go new file mode 100644 index 0000000..f6f734a --- /dev/null +++ b/credentials/credentials.go @@ -0,0 +1,112 @@ +// Copyright 2026 Block, Inc. + +// Package credentials constructs engine-neutral database credential callbacks. +// Database connection packages remain responsible for engine-specific sources, +// caching, authentication-error detection, and connection retry behavior. +package credentials + +import ( + "context" + "fmt" + "os" + + "github.com/cashapp/blip/v2" + blipaws "github.com/cashapp/blip/v2/aws" +) + +// Func returns the database credentials currently available from a configured +// source. Callers decide when to cache or refresh the result. +type Func func(context.Context) (blip.DbCredentials, error) + +// Factory constructs callbacks for credential sources shared by database +// engines. A nil password-secret parser selects Blip's default RDS secret +// parser. +type Factory struct { + awsConfig blip.AWSConfigFactory + passwordSecretParser blip.PasswordSecretParser +} + +func NewFactory(awsConfig blip.AWSConfigFactory, passwordSecretParser blip.PasswordSecretParser) Factory { + return Factory{ + awsConfig: awsConfig, + passwordSecretParser: passwordSecretParser, + } +} + +// Dynamic returns the first configured shared reloadable source in Blip's +// established precedence order: IAM, Secrets Manager, then password file. The +// boolean reports whether a source was selected. Engine-specific factories can +// insert their own sources before falling back to Static. +func (f Factory) Dynamic(cfg blip.ConfigMonitor) (Func, bool, error) { + if blip.True(cfg.AWS.IAMAuth) { + blip.Debug("%s: AWS IAM auth token password", cfg.MonitorId) + if f.awsConfig == nil { + return nil, true, fmt.Errorf("AWS IAM authentication requires an AWS config factory") + } + awscfg, err := f.awsConfig.Make(blip.AWS{Region: cfg.AWS.Region}, cfg.Hostname) + if err != nil { + return nil, true, err + } + token := blipaws.NewAuthToken(cfg.Username, cfg.Hostname, awscfg) + return func(ctx context.Context) (blip.DbCredentials, error) { + password, err := token.Password(ctx) + if err != nil { + return blip.DbCredentials{}, err + } + return blip.DbCredentials{Username: cfg.Username, Password: password}, nil + }, true, nil + } + + if cfg.AWS.PasswordSecret != "" { + blip.Debug("%s: AWS Secrets Manager password", cfg.MonitorId) + if f.awsConfig == nil { + return nil, true, fmt.Errorf("AWS Secrets Manager credentials require an AWS config factory") + } + awscfg, err := f.awsConfig.Make(blip.AWS{Region: cfg.AWS.Region}, cfg.Hostname) + if err != nil { + return nil, true, err + } + secret := blipaws.NewSecret(cfg.AWS.PasswordSecret, awscfg) + parser := f.passwordSecretParser + if parser == nil { + parser = blip.DefaultPasswordSecretParser + } + return func(ctx context.Context) (blip.DbCredentials, error) { + payload, err := secret.GetSecretPayload(ctx) + if err != nil { + return blip.DbCredentials{}, err + } + credentials := blip.DbCredentials{Username: cfg.Username} + if err := parser(ctx, cfg, payload, &credentials); err != nil { + return blip.DbCredentials{}, err + } + return credentials, nil + }, true, nil + } + + if cfg.PasswordFile != "" { + blip.Debug("%s: password file", cfg.MonitorId) + return func(context.Context) (blip.DbCredentials, error) { + contents, err := os.ReadFile(cfg.PasswordFile) + if err != nil { + return blip.DbCredentials{}, err + } + return blip.DbCredentials{Username: cfg.Username, Password: string(contents)}, nil + }, true, nil + } + + return nil, false, nil +} + +// Static returns a callback for the configured static username and password. +// It also represents passwordless authentication when Password is empty. +func Static(cfg blip.ConfigMonitor) Func { + if cfg.Password == "" { + blip.Debug("%s: no password", cfg.MonitorId) + } else { + blip.Debug("%s: static password credentials", cfg.MonitorId) + } + return func(context.Context) (blip.DbCredentials, error) { + return blip.DbCredentials{Username: cfg.Username, Password: cfg.Password}, nil + } +} diff --git a/credentials/credentials_test.go b/credentials/credentials_test.go new file mode 100644 index 0000000..755fd85 --- /dev/null +++ b/credentials/credentials_test.go @@ -0,0 +1,115 @@ +// Copyright 2026 Block, Inc. + +package credentials_test + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/cashapp/blip/v2" + "github.com/cashapp/blip/v2/credentials" +) + +func TestDynamicPasswordFileReloads(t *testing.T) { + passwordFile := filepath.Join(t.TempDir(), "password") + if err := os.WriteFile(passwordFile, []byte("first"), 0o600); err != nil { + t.Fatal(err) + } + + credentialFunc, selected, err := credentials.NewFactory(nil, nil).Dynamic(blip.ConfigMonitor{ + Username: "metrics", + PasswordFile: passwordFile, + }) + if err != nil { + t.Fatal(err) + } + if !selected { + t.Fatal("password file was not selected") + } + first, err := credentialFunc(context.Background()) + if err != nil { + t.Fatal(err) + } + if first.Username != "metrics" || first.Password != "first" { + t.Fatalf("first credentials = %+v", first) + } + + if err := os.WriteFile(passwordFile, []byte("second"), 0o600); err != nil { + t.Fatal(err) + } + second, err := credentialFunc(context.Background()) + if err != nil { + t.Fatal(err) + } + if second.Username != "metrics" || second.Password != "second" { + t.Fatalf("reloaded credentials = %+v", second) + } +} + +func TestDynamicPreservesSourcePrecedence(t *testing.T) { + iamAuth := true + tests := []struct { + name string + config blip.ConfigMonitor + wantError string + }{ + { + name: "IAM before secret and file", + config: blip.ConfigMonitor{ + PasswordFile: "/unused", + AWS: blip.ConfigAWS{ + IAMAuth: &iamAuth, + PasswordSecret: "unused", + }, + }, + wantError: "IAM authentication", + }, + { + name: "secret before file", + config: blip.ConfigMonitor{ + PasswordFile: "/unused", + AWS: blip.ConfigAWS{PasswordSecret: "unused"}, + }, + wantError: "Secrets Manager", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + credentialFunc, selected, err := credentials.NewFactory(nil, nil).Dynamic(tt.config) + if !selected { + t.Fatal("configured source was not selected") + } + if credentialFunc != nil { + t.Fatal("credential callback returned despite missing AWS factory") + } + if err == nil || !strings.Contains(err.Error(), tt.wantError) { + t.Fatalf("got error %v, expected it to contain %q", err, tt.wantError) + } + }) + } +} + +func TestDynamicReportsNoSharedSource(t *testing.T) { + credentialFunc, selected, err := credentials.NewFactory(nil, nil).Dynamic(blip.ConfigMonitor{ + Username: "metrics", + Password: "static", + }) + if err != nil || selected || credentialFunc != nil { + t.Fatalf("Dynamic returned func=%v selected=%t err=%v", credentialFunc != nil, selected, err) + } +} + +func TestStatic(t *testing.T) { + credentialFunc := credentials.Static(blip.ConfigMonitor{Username: "metrics", Password: "static"}) + got, err := credentialFunc(context.Background()) + if err != nil { + t.Fatal(err) + } + if got.Username != "metrics" || got.Password != "static" { + t.Fatalf("credentials = %+v", got) + } +} diff --git a/dbconn/credentials_test.go b/dbconn/credentials_test.go new file mode 100644 index 0000000..a939549 --- /dev/null +++ b/dbconn/credentials_test.go @@ -0,0 +1,55 @@ +// Copyright 2026 Block, Inc. + +package dbconn_test + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/cashapp/blip/v2" + "github.com/cashapp/blip/v2/dbconn" +) + +func TestCredentialsPreserveMySQLMyCnfPrecedence(t *testing.T) { + factory := dbconn.NewConnFactory(nil, nil) + credentialFunc, err := factory.Credentials(blip.ConfigMonitor{ + Username: "static-user", + Password: "static-password", + MyCnf: "../test/mycnf/full-dsn", + }) + if err != nil { + t.Fatal(err) + } + got, err := credentialFunc(context.Background()) + if err != nil { + t.Fatal(err) + } + if got.Username != "U" || got.Password != "P" { + t.Fatalf("credentials = %+v, expected my.cnf credentials", got) + } +} + +func TestCredentialsPreservePasswordFileBeforeMyCnf(t *testing.T) { + passwordFile := filepath.Join(t.TempDir(), "password") + if err := os.WriteFile(passwordFile, []byte("file-password"), 0o600); err != nil { + t.Fatal(err) + } + factory := dbconn.NewConnFactory(nil, nil) + credentialFunc, err := factory.Credentials(blip.ConfigMonitor{ + Username: "file-user", + PasswordFile: passwordFile, + MyCnf: "../test/mycnf/full-dsn", + }) + if err != nil { + t.Fatal(err) + } + got, err := credentialFunc(context.Background()) + if err != nil { + t.Fatal(err) + } + if got.Username != "file-user" || got.Password != "file-password" { + t.Fatalf("credentials = %+v, expected password-file credentials", got) + } +} diff --git a/dbconn/factory.go b/dbconn/factory.go index 407e7a3..8344654 100644 --- a/dbconn/factory.go +++ b/dbconn/factory.go @@ -19,6 +19,7 @@ import ( "github.com/cashapp/blip/v2" "github.com/cashapp/blip/v2/aws" + "github.com/cashapp/blip/v2/credentials" ) // rdsAddr matches Amazon RDS hostnames with optional :port suffix. @@ -274,46 +275,9 @@ func (f factory) Make(cfg blip.ConfigMonitor) (*sql.DB, string, error) { // credentials are fetched via a reload func, even a static credential specified // in the Blip config file. func (f factory) Credentials(cfg blip.ConfigMonitor) (CredentialFunc, error) { - - // Amazon IAM auth token (valid 15 min) - if blip.True(cfg.AWS.IAMAuth) { - blip.Debug("%s: AWS IAM auth token password", cfg.MonitorId) - awscfg, err := f.awsConfig.Make(blip.AWS{Region: cfg.AWS.Region}, cfg.Hostname) - if err != nil { - return nil, err - } - token := aws.NewAuthToken(cfg.Username, cfg.Hostname, awscfg) - return func(ctx context.Context) (blip.DbCredentials, error) { - passwd, err := token.Password(ctx) - if err != nil { - return blip.DbCredentials{}, err - } - - return blip.DbCredentials{ - Password: passwd, - Username: cfg.Username, - }, nil - }, nil - } - - // Amazon Secrets Manager, could be rotated - if cfg.AWS.PasswordSecret != "" { - return f.passwordSecretCredentialFunc(cfg) - } - - // Password file, could be "rotated" (new password written to file) - if cfg.PasswordFile != "" { - blip.Debug("%s: password file", cfg.MonitorId) - return func(context.Context) (blip.DbCredentials, error) { - bytes, err := os.ReadFile(cfg.PasswordFile) - if err != nil { - return blip.DbCredentials{}, err - } - return blip.DbCredentials{ - Password: string(bytes), - Username: cfg.Username, - }, err - }, nil + credentialFunc, selected, err := credentials.NewFactory(f.awsConfig, f.passwordSecretParser).Dynamic(cfg) + if err != nil || selected { + return credentialFunc, err } // Credentials in my.cnf file, could be rotated (username and/or password, along with TLS config) @@ -332,47 +296,7 @@ func (f factory) Credentials(cfg blip.ConfigMonitor) (CredentialFunc, error) { }, nil } - // Static password in Blip config file, not rotated - if cfg.Password != "" { - blip.Debug("%s: static password credentials", cfg.MonitorId) - return func(context.Context) (blip.DbCredentials, error) { - return blip.DbCredentials{Password: cfg.Password, Username: cfg.Username}, nil - }, nil - } - - blip.Debug("%s: no password", cfg.MonitorId) - return func(context.Context) (blip.DbCredentials, error) { - return blip.DbCredentials{Password: "", Username: cfg.Username}, nil - }, nil -} - -func (f factory) passwordSecretCredentialFunc(cfg blip.ConfigMonitor) (CredentialFunc, error) { - blip.Debug("%s: AWS Secrets Manager password", cfg.MonitorId) - awscfg, err := f.awsConfig.Make(blip.AWS{Region: cfg.AWS.Region}, cfg.Hostname) - if err != nil { - return nil, err - } - secret := aws.NewSecret(cfg.AWS.PasswordSecret, awscfg) - parser := f.passwordSecretParser - if parser == nil { - parser = blip.DefaultPasswordSecretParser - } - - return func(ctx context.Context) (blip.DbCredentials, error) { - payload, err := secret.GetSecretPayload(ctx) - if err != nil { - return blip.DbCredentials{}, err - } - - credentials := blip.DbCredentials{ - Username: cfg.Username, - } - if err := parser(ctx, cfg, payload, &credentials); err != nil { - return blip.DbCredentials{}, err - } - - return credentials, nil - }, nil + return credentials.Static(cfg), nil } // -------------------------------------------------------------------------- diff --git a/dbconn/password_secret_test.go b/dbconn/password_secret_test.go index 010c86b..e554dc4 100644 --- a/dbconn/password_secret_test.go +++ b/dbconn/password_secret_test.go @@ -69,7 +69,7 @@ func TestPasswordSecretCredentialFuncDefaultParser(t *testing.T) { defer cleanup() f := factory{awsConfig: testAWSConfigFactory{cfg: awscfg}} - credentialFunc, err := f.passwordSecretCredentialFunc(blip.ConfigMonitor{ + credentialFunc, err := f.Credentials(blip.ConfigMonitor{ Hostname: "db.example.com", Username: "config-user", AWS: blip.ConfigAWS{ @@ -116,7 +116,7 @@ func TestPasswordSecretCredentialFuncCustomParser(t *testing.T) { return nil }, } - credentialFunc, err := f.passwordSecretCredentialFunc(blip.ConfigMonitor{ + credentialFunc, err := f.Credentials(blip.ConfigMonitor{ Hostname: "db.example.com", Username: "config-user", AWS: blip.ConfigAWS{ @@ -154,7 +154,7 @@ func TestPasswordSecretCredentialFuncParserError(t *testing.T) { return parseErr }, } - credentialFunc, err := f.passwordSecretCredentialFunc(blip.ConfigMonitor{ + credentialFunc, err := f.Credentials(blip.ConfigMonitor{ Hostname: "db.example.com", Username: "config-user", AWS: blip.ConfigAWS{ @@ -187,7 +187,7 @@ func TestPasswordSecretCredentialFuncGetSecretError(t *testing.T) { }, }}, } - credentialFunc, err := f.passwordSecretCredentialFunc(blip.ConfigMonitor{ + credentialFunc, err := f.Credentials(blip.ConfigMonitor{ Hostname: "db.example.com", Username: "config-user", AWS: blip.ConfigAWS{ @@ -209,7 +209,7 @@ func TestPasswordSecretCredentialFuncAWSConfigError(t *testing.T) { configErr := errors.New("aws config") f := factory{awsConfig: testAWSConfigFactory{err: configErr}} - _, err := f.passwordSecretCredentialFunc(blip.ConfigMonitor{ + _, err := f.Credentials(blip.ConfigMonitor{ Hostname: "db.example.com", Username: "config-user", AWS: blip.ConfigAWS{ diff --git a/dbconn/reload_password.go b/dbconn/reload_password.go index 27af381..c7805e5 100644 --- a/dbconn/reload_password.go +++ b/dbconn/reload_password.go @@ -10,6 +10,7 @@ import ( "github.com/go-sql-driver/mysql" "github.com/cashapp/blip/v2" + "github.com/cashapp/blip/v2/credentials" "github.com/cashapp/blip/v2/event" ) @@ -17,7 +18,9 @@ func init() { dsndriver.SetHotswapFunc(Repo.ReloadDSN) } -type CredentialFunc func(context.Context) (blip.DbCredentials, error) +// CredentialFunc is retained as an alias for compatibility with integrations +// that used the MySQL dbconn package to construct credential callbacks. +type CredentialFunc = credentials.Func type repo struct { m *sync.Map From b747fa7535396609b16b93a69174f6bfbb2b7019 Mon Sep 17 00:00:00 2001 From: Ian Oberst Date: Thu, 23 Jul 2026 09:27:07 -0700 Subject: [PATCH 2/5] Add database-aware metric collectors Read database compatibility from an optional collector factory capability while defaulting legacy factories to MySQL. Reject plans whose collectors have no common database type, then validate the selected plan against each monitor before collector preparation. Co-authored-by: Codex Ai-assisted: true --- collector.go | 25 +++- docs/content/develop/collectors.md | 14 +- docs/content/plans/file.md | 6 + metrics/factory.go | 129 +++++++++++++++++-- metrics/factory_test.go | 197 +++++++++++++++++++++++++++++ plan/loader.go | 109 +++++++++++++++- plan/loader_test.go | 148 ++++++++++++++++++++++ 7 files changed, 610 insertions(+), 18 deletions(-) create mode 100644 metrics/factory_test.go diff --git a/collector.go b/collector.go index e626e44..6d7cec3 100644 --- a/collector.go +++ b/collector.go @@ -22,8 +22,8 @@ type Collector interface { // Prepare prepares a plan for future calls to Collect. The return function // is called once when the collector is destroyed; it allows the collector // to clean up. If Prepare returns an error, Blip will retry preparing the - // plan. Therefore, Prepare should not retry on error (for example, if MySQL - // is not online yet). + // plan. Therefore, Prepare should not retry on error (for example, if the + // database is not online yet). Prepare(ctx context.Context, plan Plan) (func(), error) // Collect collects metrics for the previously prepared plan. Collect is only @@ -111,13 +111,14 @@ func (h CollectorHelp) Validate(opts map[string]string) error { // a Collector. The factory must use the args to create the collector. type CollectorFactoryArgs struct { // Config is the full and final monitor config. Most collectors do not need - // this, but some that collect metrics outside MySQL, like cloud metrics, + // this, but some that collect metrics outside the database, like cloud metrics, // might need additional monitor config values. Config ConfigMonitor - // DB is the connection to MySQL. It is safe for concurrent use, and it is - // used concurrently by other parts of a monitor. The Collector must not - // modify the connection, reconnect, and so forth--only use the connection. + // DB is the monitor's database connection. It is safe for concurrent use, + // and it is used concurrently by other parts of a monitor. The Collector + // must not modify the connection, reconnect, and so forth--only use the + // connection. DB *sql.DB // MonitorId is the monitor identifier. The Collector must include @@ -135,5 +136,17 @@ type CollectorFactory interface { Make(domain string, args CollectorFactoryArgs) (Collector, error) } +// CollectorFactoryDatabaseTypes is an optional CollectorFactory capability +// that identifies the database types supported by a domain. Factories that do +// not implement this interface retain Blip's historical MySQL behavior. +// +// The domain argument allows one factory to serve domains with different +// compatibility, such as MySQL collectors and database-neutral cloud metrics. +// Implementations must return at least one database type. +type CollectorFactoryDatabaseTypes interface { + CollectorFactory + DatabaseTypes(domain string) []DatabaseType +} + // ErrMore signals that a collector will return more values. See https://block.github.io/blip/develop/collectors/#long-running. var ErrMore = errors.New("more metrics") diff --git a/docs/content/develop/collectors.md b/docs/content/develop/collectors.md index 79a476d..807b2b0 100644 --- a/docs/content/develop/collectors.md +++ b/docs/content/develop/collectors.md @@ -114,7 +114,7 @@ Example [https://github.com/cashapp/blip/tree/main/examples/integrate](https://g The high-level work is: 1. Implement `blip.Collector` and `blip.CollectorFactory` -2. Register the domain/collector by calling `metrics.Register(myFactory, "foo")` +2. Register the domain/collector by calling `metrics.Register("foo", myFactory)` 3. Use the domain in a plan: ```yaml @@ -125,6 +125,18 @@ level: - whatever ``` +`metrics.Register` preserves the original integration behavior: a factory that +only implements `CollectorFactory` is treated as MySQL. A factory for another +database type implements the optional `CollectorFactoryDatabaseTypes` +capability so Blip can reject an incompatible monitor plan before collector +preparation: + +```go +func (myFactory) DatabaseTypes(string) []blip.DatabaseType { + return []blip.DatabaseType{blip.DatabaseTypePostgres} +} +``` + ## Long-running As of Blip v1.2.0, long-running collectors are possible using one of two approaches: diff --git a/docs/content/plans/file.md b/docs/content/plans/file.md index 42d98a3..7e4eb4a 100644 --- a/docs/content/plans/file.md +++ b/docs/content/plans/file.md @@ -49,6 +49,12 @@ And each domain has a domain-specific configuration that includes: These values are documented for each [domain]({{< ref "/metrics/domains" >}}) and printed on the command line by [`--print-domains`]({{< ref "/config/blip#--print-domains" >}}). +All domains in a plan, across every level, must support at least one common +database type. A MySQL-only domain and a domain that supports both MySQL and +PostgreSQL can share a plan, but a MySQL-only domain and a PostgreSQL-only +domain cannot. A shared plan configuration can still contain separate plans +for different database types. + Since Blip automatically levels up overlapping frequencies (described in [Intro / Plans]({{< ref "intro/plans" >}})), it's conventional to define levels from most to least frequent, as in this example: ```yaml diff --git a/metrics/factory.go b/metrics/factory.go index d3f4d47..6899fef 100644 --- a/metrics/factory.go +++ b/metrics/factory.go @@ -28,25 +28,75 @@ import ( waitiotable "github.com/cashapp/blip/v2/metrics/wait.io.table" ) -// Register registers a factory that makes one or more collector by domain name. -// This is function is one several integration points because it allows users +// Register registers a factory that makes one or more collectors by domain name. +// This is one of several integration points because it allows users // to plug in new metric collectors by providing a factory to make them. // Blip calls this function in an init function to register the built-in metric // collectors. // +// If the factory implements blip.CollectorFactoryDatabaseTypes, Blip records +// the database types it declares for this domain. Existing factories that do +// not implement that optional interface retain Blip's historical MySQL +// behavior. +// // See types in the blip package for more details. func Register(domain string, f blip.CollectorFactory) error { + r.Lock() + _, registered := r.factory[domain] + r.Unlock() + if registered { + return fmt.Errorf("%s already registered", domain) + } + + databaseTypes := []blip.DatabaseType{blip.DatabaseTypeMySQL} + if typedFactory, ok := f.(blip.CollectorFactoryDatabaseTypes); ok { + databaseTypes = typedFactory.DatabaseTypes(domain) + } + databaseTypes, err := normalizeDatabaseTypes(domain, databaseTypes) + if err != nil { + return err + } + r.Lock() defer r.Unlock() - _, ok := r.factory[domain] - if ok { + // Another goroutine might have registered the domain while the factory's + // optional compatibility metadata was being evaluated. + if _, registered := r.factory[domain]; registered { return fmt.Errorf("%s already registered", domain) } - r.factory[domain] = f - blip.Debug("register collector %s", domain) + r.factory[domain] = registeredFactory{ + factory: f, + databaseTypes: databaseTypes, + } + blip.Debug("register collector %s for database types %v", domain, databaseTypes) return nil } +func normalizeDatabaseTypes(domain string, databaseTypes []blip.DatabaseType) ([]blip.DatabaseType, error) { + if len(databaseTypes) == 0 { + return nil, fmt.Errorf("collector %s supports no database types", domain) + } + + seen := map[blip.DatabaseType]bool{} + normalized := make([]blip.DatabaseType, 0, len(databaseTypes)) + for _, databaseType := range databaseTypes { + switch databaseType { + case blip.DatabaseTypeMySQL, blip.DatabaseTypePostgres: + default: + return nil, fmt.Errorf("collector %s declares invalid database type %q", domain, databaseType) + } + if seen[databaseType] { + continue + } + seen[databaseType] = true + normalized = append(normalized, databaseType) + } + sort.Slice(normalized, func(i, j int) bool { + return normalized[i] < normalized[j] + }) + return normalized, nil +} + // Remove removes the metrics collector factory for the given domain. This is // used for testing, but it can also be used to remove (or override) built-in // metric collectors. @@ -77,18 +127,61 @@ func Exists(domain string) bool { return ok } +// SupportedDatabaseTypes returns a copy of the database types supported by the +// registered collector domain. +func SupportedDatabaseTypes(domain string) ([]blip.DatabaseType, error) { + r.Lock() + defer r.Unlock() + registered, ok := r.factory[domain] + if !ok { + return nil, fmt.Errorf("invalid domain: %s (no factory registered)", domain) + } + databaseTypes := make([]blip.DatabaseType, len(registered.databaseTypes)) + copy(databaseTypes, registered.databaseTypes) + return databaseTypes, nil +} + +// ValidateDatabase returns nil if the domain exists and can be used with the +// database type. +func ValidateDatabase(domain string, databaseType blip.DatabaseType) error { + r.Lock() + defer r.Unlock() + return validateDatabase(domain, databaseType) +} + +func validateDatabase(domain string, databaseType blip.DatabaseType) error { + registered, ok := r.factory[domain] + if !ok { + return fmt.Errorf("invalid domain: %s (no factory registered)", domain) + } + for _, supportedType := range registered.databaseTypes { + if supportedType == databaseType { + return nil + } + } + return fmt.Errorf("collector %s does not support database type %q (supported: %v)", + domain, databaseType, registered.databaseTypes) +} + // Make makes a metric collector for the domain using a previously registered factory. // // See types in the blip package for more details. func Make(domain string, args blip.CollectorFactoryArgs) (blip.Collector, error) { r.Lock() defer r.Unlock() - f, ok := r.factory[domain] + registered, ok := r.factory[domain] if !ok { return nil, fmt.Errorf("invalid domain: %s (no factory registered)", domain) } - return f.Make(domain, args) + // ValidatePlans creates collectors without a monitor. Database compatibility + // is checked when the monitor resolves the selected plan. + if !args.Validate { + if err := validateDatabase(domain, args.Config.EffectiveDatabaseType()); err != nil { + return nil, err + } + } + return registered.factory.Make(domain, args) } func PrintDomains() string { @@ -216,14 +309,19 @@ func init() { // instance below. type repo struct { *sync.Mutex - factory map[string]blip.CollectorFactory + factory map[string]registeredFactory +} + +type registeredFactory struct { + factory blip.CollectorFactory + databaseTypes []blip.DatabaseType } // Internal package instance of repo that holds all collector factories registered // by calls to Register, which includes the built-in factories. var r = &repo{ Mutex: &sync.Mutex{}, - factory: map[string]blip.CollectorFactory{}, + factory: map[string]registeredFactory{}, } // factory is the built-in factory for creating all built-in collectors. @@ -234,6 +332,7 @@ type factory struct { } var _ blip.CollectorFactory = &factory{} +var _ blip.CollectorFactoryDatabaseTypes = &factory{} // Internet package instance of factory that makes all built-it collectors. // This factory is registered in the init func above. @@ -244,6 +343,16 @@ func InitFactory(factories blip.Factories) { f.HTTPClient = factories.HTTPClient } +func (f *factory) DatabaseTypes(domain string) []blip.DatabaseType { + if domain == awsrds.DOMAIN { + return []blip.DatabaseType{ + blip.DatabaseTypeMySQL, + blip.DatabaseTypePostgres, + } + } + return []blip.DatabaseType{blip.DatabaseTypeMySQL} +} + // Make makes a metric collector for the domain. This is the built-in factory // that makes the built-in collectors: status.global, var.global, and so on. func (f *factory) Make(domain string, args blip.CollectorFactoryArgs) (blip.Collector, error) { diff --git a/metrics/factory_test.go b/metrics/factory_test.go new file mode 100644 index 0000000..8b60c64 --- /dev/null +++ b/metrics/factory_test.go @@ -0,0 +1,197 @@ +// Copyright 2026 Block, Inc. + +package metrics_test + +import ( + "strings" + "testing" + + "github.com/cashapp/blip" + "github.com/cashapp/blip/metrics" + "github.com/cashapp/blip/test/mock" +) + +type databaseTypesFactory struct { + mock.MetricFactory + databaseTypes func(string) []blip.DatabaseType +} + +func (f databaseTypesFactory) DatabaseTypes(domain string) []blip.DatabaseType { + return f.databaseTypes(domain) +} + +func TestRegisterDefaultsToMySQL(t *testing.T) { + const domain = "test.legacy-mysql" + factory := mock.MetricFactory{} + + if err := metrics.Register(domain, factory); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(domain) }) + + if err := metrics.ValidateDatabase(domain, blip.DatabaseTypeMySQL); err != nil { + t.Fatalf("ValidateDatabase(mysql): %v", err) + } + if _, err := metrics.Make(domain, blip.CollectorFactoryArgs{}); err != nil { + t.Fatalf("Make(default mysql): %v", err) + } + + err := metrics.ValidateDatabase(domain, blip.DatabaseTypePostgres) + if err == nil || !strings.Contains(err.Error(), `does not support database type "postgres" (supported: [mysql])`) { + t.Fatalf("ValidateDatabase(postgres) error = %v", err) + } +} + +func TestRegisterUsesFactoryDatabaseTypes(t *testing.T) { + const domain = "test.postgres-only" + factory := databaseTypesFactory{ + databaseTypes: func(gotDomain string) []blip.DatabaseType { + if gotDomain != domain { + t.Fatalf("DatabaseTypes domain = %q, expected %q", gotDomain, domain) + } + return []blip.DatabaseType{blip.DatabaseTypePostgres} + }, + } + + if err := metrics.Register(domain, factory); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(domain) }) + + if err := metrics.ValidateDatabase(domain, blip.DatabaseTypePostgres); err != nil { + t.Fatalf("ValidateDatabase(postgres): %v", err) + } + if _, err := metrics.Make(domain, blip.CollectorFactoryArgs{ + Config: blip.ConfigMonitor{DatabaseType: blip.DatabaseTypePostgres}, + }); err != nil { + t.Fatalf("Make(postgres): %v", err) + } + + err := metrics.ValidateDatabase(domain, blip.DatabaseTypeMySQL) + if err == nil || !strings.Contains(err.Error(), `does not support database type "mysql" (supported: [postgres])`) { + t.Fatalf("ValidateDatabase(mysql) error = %v", err) + } + if _, err := metrics.Make(domain, blip.CollectorFactoryArgs{}); err == nil { + t.Fatal("Make with the default MySQL database type succeeded") + } + + // Global plan validation has no monitor database type. It must still be + // able to construct the collector so shared plans can be loaded. + if _, err := metrics.Make(domain, blip.CollectorFactoryArgs{Validate: true}); err != nil { + t.Fatalf("Make(validate): %v", err) + } +} + +func TestRegisterSupportsMultipleDatabaseTypes(t *testing.T) { + const domain = "test.multiple-database-types" + factory := databaseTypesFactory{ + databaseTypes: func(string) []blip.DatabaseType { + return []blip.DatabaseType{ + blip.DatabaseTypePostgres, + blip.DatabaseTypeMySQL, + blip.DatabaseTypePostgres, + } + }, + } + + if err := metrics.Register(domain, factory); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(domain) }) + + for _, databaseType := range []blip.DatabaseType{ + blip.DatabaseTypeMySQL, + blip.DatabaseTypePostgres, + } { + if err := metrics.ValidateDatabase(domain, databaseType); err != nil { + t.Fatalf("ValidateDatabase(%s): %v", databaseType, err) + } + } + + databaseTypes, err := metrics.SupportedDatabaseTypes(domain) + if err != nil { + t.Fatal(err) + } + if len(databaseTypes) != 2 || + databaseTypes[0] != blip.DatabaseTypeMySQL || + databaseTypes[1] != blip.DatabaseTypePostgres { + t.Fatalf("SupportedDatabaseTypes = %v", databaseTypes) + } + + // The registry owns its normalized compatibility metadata. + databaseTypes[0] = "mutated" + if err := metrics.ValidateDatabase(domain, blip.DatabaseTypeMySQL); err != nil { + t.Fatalf("ValidateDatabase(mysql) after returned slice mutation: %v", err) + } +} + +func TestRegisterValidatesFactoryDatabaseTypes(t *testing.T) { + tests := map[string]struct { + databaseTypes []blip.DatabaseType + errorContains string + }{ + "empty": { + databaseTypes: nil, + errorContains: "supports no database types", + }, + "invalid": { + databaseTypes: []blip.DatabaseType{"oracle"}, + errorContains: `declares invalid database type "oracle"`, + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + domain := "test.invalid-database-types-" + name + factory := databaseTypesFactory{ + databaseTypes: func(string) []blip.DatabaseType { + return tt.databaseTypes + }, + } + + err := metrics.Register(domain, factory) + if err == nil || !strings.Contains(err.Error(), tt.errorContains) { + t.Fatalf("Register error = %v", err) + } + if metrics.Exists(domain) { + t.Fatalf("%s was registered", domain) + } + }) + } +} + +func TestRegisterDuplicateDoesNotInspectFactoryDatabaseTypes(t *testing.T) { + const domain = "test.duplicate-database-types" + if err := metrics.Register(domain, mock.MetricFactory{}); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(domain) }) + + factory := databaseTypesFactory{ + databaseTypes: func(string) []blip.DatabaseType { + t.Fatal("DatabaseTypes called for duplicate registration") + return nil + }, + } + err := metrics.Register(domain, factory) + if err == nil || !strings.Contains(err.Error(), "already registered") { + t.Fatalf("Register duplicate error = %v", err) + } +} + +func TestBuiltInCollectorDatabaseCompatibility(t *testing.T) { + if err := metrics.ValidateDatabase("status.global", blip.DatabaseTypeMySQL); err != nil { + t.Fatalf("status.global with MySQL: %v", err) + } + if err := metrics.ValidateDatabase("status.global", blip.DatabaseTypePostgres); err == nil { + t.Fatal("status.global supports PostgreSQL") + } + + for _, databaseType := range []blip.DatabaseType{ + blip.DatabaseTypeMySQL, + blip.DatabaseTypePostgres, + } { + if err := metrics.ValidateDatabase("aws.rds", databaseType); err != nil { + t.Fatalf("aws.rds with %s: %v", databaseType, err) + } + } +} diff --git a/plan/loader.go b/plan/loader.go index 62c481f..00eefb6 100644 --- a/plan/loader.go +++ b/plan/loader.go @@ -9,6 +9,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "sync" "time" @@ -36,6 +37,7 @@ type Loader struct { plugin func(blip.ConfigPlans) ([]blip.Plan, error) sharedPlans []Meta // keyed on Plan.Name monitorPlans map[string][]Meta // keyed on monitorId, Plan.Name + monitorTypes map[string]blip.DatabaseType *sync.RWMutex } @@ -44,6 +46,7 @@ func NewLoader(plugin func(blip.ConfigPlans) ([]blip.Plan, error)) *Loader { plugin: plugin, sharedPlans: []Meta{}, monitorPlans: map[string][]Meta{}, + monitorTypes: map[string]blip.DatabaseType{}, RWMutex: &sync.RWMutex{}, } } @@ -173,6 +176,13 @@ func (pl *Loader) LoadShared(cfg blip.ConfigPlans, dbMaker blip.DbFactory) error func (pl *Loader) LoadMonitor(mon blip.ConfigMonitor, dbMaker blip.DbFactory) error { event.Sendf(event.PLANS_LOAD_MONITOR, "%s", mon.MonitorId) + // Remember the type even when the monitor uses only shared plans. Global + // plan loading validates domains and options; compatibility is validated + // after this monitor selects one of those plans. + pl.Lock() + pl.monitorTypes[mon.MonitorId] = mon.EffectiveDatabaseType() + pl.Unlock() + if mon.Plans.Table == "" && len(mon.Plans.Files) == 0 { blip.Debug("monitor %s uses only shared plans", mon.MonitorId) return nil @@ -286,7 +296,16 @@ func (pl *Loader) Plan(monitorId string, planName string, db *sql.DB) (blip.Plan blip.Debug("%s: loading plan %s from %s", monitorId, planName, pm.Source) // Since blip.Plan has field types that pass by reference (maps and slices), we want to the returned plan to // be a deep copy to ensure the caller cannot modify the original shared plan. - return deepcopyPlan(&pm.plan) + loadedPlan, err := deepcopyPlan(&pm.plan) + if err != nil { + return blip.Plan{}, err + } + if databaseType, ok := pl.monitorTypes[monitorId]; ok { + if err := ValidatePlanDatabase(loadedPlan, databaseType); err != nil { + return blip.Plan{}, fmt.Errorf("monitor %s: %w", monitorId, err) + } + } + return loadedPlan, nil } func (pl *Loader) SharedPlans() []Meta { @@ -514,6 +533,8 @@ func ValidatePlans(plans []blip.Plan) error { continue } + validDomains := true + // Second level validation: PlanLoader checks that domains exist, and // domain options vs collector help for levelName := range plans[i].Levels { @@ -532,6 +553,7 @@ func ValidatePlans(plans []blip.Plan) error { var err error mc, err = metrics.Make(domainName, blip.CollectorFactoryArgs{Validate: true}) if err != nil { + validDomains = false errMsgs = append(errMsgs, fmt.Sprintf("invalid plan: %s: at %s/%s: %s", plans[i].Name, levelName, domainName, err)) continue DOMAINS @@ -551,6 +573,12 @@ func ValidatePlans(plans []blip.Plan) error { } } } + + if validDomains { + if err := validatePlanDatabaseCompatibility(plans[i]); err != nil { + errMsgs = append(errMsgs, fmt.Sprintf("invalid plan: %s: %s", plans[i].Name, err)) + } + } } // Third level validation is each collector Prepare, called by monitor/Engine.Prepare @@ -562,6 +590,85 @@ func ValidatePlans(plans []blip.Plan) error { return nil } +func validatePlanDatabaseCompatibility(plan blip.Plan) error { + domainSet := map[string]struct{}{} + for _, level := range plan.Levels { + for domain := range level.Collect { + domainSet[domain] = struct{}{} + } + } + if len(domainSet) == 0 { + return nil + } + + domains := make([]string, 0, len(domainSet)) + for domain := range domainSet { + domains = append(domains, domain) + } + sort.Strings(domains) + + commonTypes := map[blip.DatabaseType]bool{} + domainTypes := make([]string, 0, len(domains)) + for i, domain := range domains { + supportedTypes, err := metrics.SupportedDatabaseTypes(domain) + if err != nil { + return err + } + domainTypes = append(domainTypes, fmt.Sprintf("%s=%v", domain, supportedTypes)) + + supported := map[blip.DatabaseType]bool{} + for _, databaseType := range supportedTypes { + supported[databaseType] = true + if i == 0 { + commonTypes[databaseType] = true + } + } + if i == 0 { + continue + } + for databaseType := range commonTypes { + if !supported[databaseType] { + delete(commonTypes, databaseType) + } + } + } + + if len(commonTypes) == 0 { + return fmt.Errorf("collectors have no common database type: %s", strings.Join(domainTypes, ", ")) + } + return nil +} + +// ValidatePlanDatabase returns nil if every collector in the plan supports the +// monitor's database type. Global plan loading cannot perform this validation +// because one set of shared plans can serve monitors of different types. +func ValidatePlanDatabase(plan blip.Plan, databaseType blip.DatabaseType) error { + domainSet := map[string]struct{}{} + for _, level := range plan.Levels { + for domain := range level.Collect { + domainSet[domain] = struct{}{} + } + } + + domains := make([]string, 0, len(domainSet)) + for domain := range domainSet { + domains = append(domains, domain) + } + sort.Strings(domains) + + errMsgs := []string{} + for _, domain := range domains { + if err := metrics.ValidateDatabase(domain, databaseType); err != nil { + errMsgs = append(errMsgs, err.Error()) + } + } + if len(errMsgs) > 0 { + return fmt.Errorf("plan %s is incompatible with database type %q:\n%s", + plan.Name, databaseType, strings.Join(errMsgs, "\n")) + } + return nil +} + func deepcopyPlan(p *blip.Plan) (blip.Plan, error) { // Since the operation needed here is not performance critical we can piggy back off of gob // encode/decode to make the deep copy and not rely on staying up-to-date with the blip.Plan's diff --git a/plan/loader_test.go b/plan/loader_test.go index 21ff54a..c0da20d 100644 --- a/plan/loader_test.go +++ b/plan/loader_test.go @@ -6,6 +6,7 @@ import ( "context" "fmt" "path/filepath" + "strings" "testing" "github.com/go-test/deep" @@ -164,3 +165,150 @@ func TestPlanShouldReturnDeepCopyOfPlan(t *testing.T) { } } } + +type planDatabaseTypesFactory struct { + mock.MetricFactory + databaseTypes []blip.DatabaseType +} + +func (f planDatabaseTypesFactory) DatabaseTypes(string) []blip.DatabaseType { + return f.databaseTypes +} + +func TestSharedPlansValidateDatabaseCompatibilityPerMonitor(t *testing.T) { + const ( + mysqlDomain = "test.mysql-plan" + postgresDomain = "test.postgres-plan" + sharedDomain = "test.shared-plan" + ) + factory := mock.MetricFactory{} + if err := metrics.Register(mysqlDomain, factory); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(mysqlDomain) }) + if err := metrics.Register(postgresDomain, planDatabaseTypesFactory{ + databaseTypes: []blip.DatabaseType{blip.DatabaseTypePostgres}, + }); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(postgresDomain) }) + if err := metrics.Register(sharedDomain, planDatabaseTypesFactory{ + databaseTypes: []blip.DatabaseType{ + blip.DatabaseTypeMySQL, + blip.DatabaseTypePostgres, + }, + }); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(sharedDomain) }) + + newPlan := func(name, databaseDomain string) blip.Plan { + return blip.Plan{ + Name: name, + Levels: map[string]blip.Level{ + "level": { + Name: "level", + Freq: "1s", + Collect: map[string]blip.Domain{ + databaseDomain: {}, + sharedDomain: {}, + }, + }, + }, + } + } + mysqlPlan := newPlan("mysql-plan", mysqlDomain) + postgresPlan := newPlan("postgres-plan", postgresDomain) + + pl := plan.NewLoader(func(blip.ConfigPlans) ([]blip.Plan, error) { + return []blip.Plan{mysqlPlan, postgresPlan}, nil + }) + if err := pl.LoadShared(blip.ConfigPlans{}, nil); err != nil { + t.Fatalf("LoadShared: %v", err) + } + + // The omitted database type exercises Blip's existing MySQL default. + if err := pl.LoadMonitor(blip.ConfigMonitor{MonitorId: "mysql"}, nil); err != nil { + t.Fatalf("LoadMonitor(mysql): %v", err) + } + if err := pl.LoadMonitor(blip.ConfigMonitor{ + MonitorId: "postgres", + DatabaseType: blip.DatabaseTypePostgres, + }, nil); err != nil { + t.Fatalf("LoadMonitor(postgres): %v", err) + } + + if _, err := pl.Plan("mysql", mysqlPlan.Name, nil); err != nil { + t.Fatalf("MySQL plan for MySQL monitor: %v", err) + } + if _, err := pl.Plan("postgres", postgresPlan.Name, nil); err != nil { + t.Fatalf("PostgreSQL plan for PostgreSQL monitor: %v", err) + } + + if _, err := pl.Plan("mysql", postgresPlan.Name, nil); err == nil || + !strings.Contains(err.Error(), `collector test.postgres-plan does not support database type "mysql" (supported: [postgres])`) { + t.Fatalf("PostgreSQL plan for MySQL monitor error = %v", err) + } + if _, err := pl.Plan("postgres", mysqlPlan.Name, nil); err == nil || + !strings.Contains(err.Error(), `collector test.mysql-plan does not support database type "postgres" (supported: [mysql])`) { + t.Fatalf("MySQL plan for PostgreSQL monitor error = %v", err) + } +} + +func TestValidatePlansRejectsCollectorsWithoutCommonDatabaseType(t *testing.T) { + const ( + mysqlDomain = "test.no-common-mysql" + postgresDomain = "test.no-common-postgres" + sharedDomain = "test.no-common-shared" + ) + if err := metrics.Register(mysqlDomain, mock.MetricFactory{}); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(mysqlDomain) }) + if err := metrics.Register(postgresDomain, planDatabaseTypesFactory{ + databaseTypes: []blip.DatabaseType{blip.DatabaseTypePostgres}, + }); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(postgresDomain) }) + if err := metrics.Register(sharedDomain, planDatabaseTypesFactory{ + databaseTypes: []blip.DatabaseType{ + blip.DatabaseTypeMySQL, + blip.DatabaseTypePostgres, + }, + }); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(sharedDomain) }) + + mixedPlan := blip.Plan{ + Name: "mixed-database-plan", + Levels: map[string]blip.Level{ + "mysql": { + Freq: "1s", + Collect: map[string]blip.Domain{ + mysqlDomain: {}, + sharedDomain: {}, + }, + }, + "postgres": { + Freq: "5s", + Collect: map[string]blip.Domain{ + postgresDomain: {}, + }, + }, + }, + } + + err := plan.ValidatePlans([]blip.Plan{mixedPlan}) + if err == nil { + t.Fatal("mixed MySQL and PostgreSQL plan is valid") + } + expected := "collectors have no common database type: " + + "test.no-common-mysql=[mysql], " + + "test.no-common-postgres=[postgres], " + + "test.no-common-shared=[mysql postgres]" + if !strings.Contains(err.Error(), expected) { + t.Fatalf("ValidatePlans error = %v", err) + } +} From ae286622e8ee28b63138a336edbce791fbbf2a0c Mon Sep 17 00:00:00 2001 From: Ian Oberst Date: Fri, 7 Aug 2026 09:41:08 -0700 Subject: [PATCH 3/5] Add PostgreSQL multi-database monitoring (#177) * Configure PostgreSQL database discovery Co-authored-by: Codex Ai-assisted: true Co-authored-by: Goose * Add monitor-owned database providers Co-authored-by: Codex Ai-assisted: true * Keep provider constructors internal Co-authored-by: Codex Ai-assisted: true * Harden PostgreSQL monitor lifecycle (#174) * Guard PostgreSQL monitor configuration Keep MySQL-only heartbeat and plan defaults off PostgreSQL monitors, reject explicit unsupported settings, and sign IAM tokens with the database-specific default port. Co-authored-by: Codex Ai-assisted: true * Close PostgreSQL integration gaps Cancel and join plan preparation before closing monitor-owned database resources, and reject the remaining MySQL-only plan configurations for PostgreSQL. Co-authored-by: Codex Ai-assisted: true * Require PostgreSQL exporter plans Avoid assigning the MySQL default exporter plan to PostgreSQL monitors. Require a named plan whenever exporter mode is enabled for PostgreSQL while preserving the public and MySQL defaulting behavior. Co-authored-by: Codex Ai-assisted: true * Coordinate monitor subsystem teardown Roll back partial startup failures, bind subsystem stops to their startup generation, and retain exporter engines for cleanup before database providers close. Collector cleanup now also runs when a prepared collector is idle. Co-authored-by: Codex Ai-assisted: true * Join collectors before provider shutdown Track every engine collector goroutine, cancel active runs during teardown, and wait for foreground or ErrMore background work before invoking cleanup and releasing the database provider. Co-authored-by: Codex Ai-assisted: true * Generalize external database modules (#175) * Generalize external database modules Co-authored-by: Codex Ai-assisted: true * Interpolate typed database config values Preserve named and typed containers when Blip expands environment and monitor placeholders in opaque module configuration. This keeps programmatic config loaders consistent with YAML-loaded config. Co-authored-by: Codex Ai-assisted: true --------- Co-authored-by: Codex --------- Co-authored-by: Codex --------- Co-authored-by: Codex Co-authored-by: Goose --- aws/iam_auth.go | 10 +- blip.go | 23 +- collector.go | 12 +- config.go | 152 +++--- config_database_test.go | 536 +++++++++++++++++++++ config_postgres.go | 187 -------- config_postgres_test.go | 179 ------- credentials/credentials.go | 10 +- credentials/credentials_test.go | 104 +++- database_module.go | 210 ++++++++ dbconn/factory.go | 2 +- docs/content/config/config-file.md | 23 +- docs/content/config/heartbeat.md | 5 + docs/content/develop/collectors.md | 6 +- docs/content/develop/database-modules.md | 77 +++ docs/content/plans/changing.md | 5 + docs/content/plans/file.md | 6 +- docs/content/plans/table.md | 7 + metrics/factory.go | 30 +- metrics/factory_test.go | 145 +++++- monitor/engine.go | 64 ++- monitor/level_collector.go | 65 ++- monitor/monitor.go | 159 ++++-- monitor/mysqld_exporter.go | 10 + monitor/provider_test.go | 585 +++++++++++++++++++++++ plan/loader.go | 13 +- plan/loader_test.go | 87 ++-- 27 files changed, 2125 insertions(+), 587 deletions(-) create mode 100644 config_database_test.go delete mode 100644 config_postgres.go delete mode 100644 config_postgres_test.go create mode 100644 database_module.go create mode 100644 docs/content/develop/database-modules.md create mode 100644 monitor/provider_test.go diff --git a/aws/iam_auth.go b/aws/iam_auth.go index 4e70e6f..1cffefb 100644 --- a/aws/iam_auth.go +++ b/aws/iam_auth.go @@ -19,9 +19,15 @@ type AuthToken struct { } func NewAuthToken(username, hostname string, cfg aws.Config) AuthToken { - // RDS auth tokens require the :3306 suffix + return NewAuthTokenWithDefaultPort(username, hostname, "3306", cfg) +} + +// NewAuthTokenWithDefaultPort constructs an RDS authentication token signer, +// adding defaultPort when hostname does not already include a port. +func NewAuthTokenWithDefaultPort(username, hostname, defaultPort string, cfg aws.Config) AuthToken { + // RDS auth tokens require the database port in the signed endpoint. if !portRe.MatchString(hostname) { - hostname += ":3306" + hostname += ":" + defaultPort } return AuthToken{ diff --git a/blip.go b/blip.go index 5db5fff..8cac6d4 100644 --- a/blip.go +++ b/blip.go @@ -34,11 +34,11 @@ const ( EVENT ) -// Metrics are metrics collected for one plan level, from one MySQL instance. +// Metrics are metrics collected for one plan level, from one monitor. type Metrics struct { Begin time.Time // when collection started End time.Time // when collection completed - MonitorId string // ID of monitor (MySQL) + MonitorId string // ID of monitor Plan string // plan name Level string // level name Interval uint // interval number @@ -99,7 +99,7 @@ type SinkFactoryArgs struct { Tags map[string]string // config.monitor.tags } -// DbCredentials are MySQL credentials parsed or loaded for a connection. +// DbCredentials are database credentials parsed or loaded for a connection. type DbCredentials struct { Username string Password string @@ -218,6 +218,23 @@ type DbFactory interface { Make(ConfigMonitor) (*sql.DB, string, error) } +// DbProvider owns the database connections associated with one monitor. +// Primary returns the connection used by existing Blip subsystems and +// collectors. Close releases the primary connection and any additional +// database-specific resources owned by the provider. +type DbProvider interface { + Primary() *sql.DB + Close() error +} + +// DbProviderFactory is an optional DbFactory capability for database engines +// that need to own more than one connection pool per monitor. Blip preserves +// the existing DbFactory.Make path for factories that do not implement it. +type DbProviderFactory interface { + DbFactory + MakeProvider(ConfigMonitor) (DbProvider, string, error) +} + type HTTPClientFactory interface { MakeForSink(sinkName, monitorId string, opts, tags map[string]string) (*http.Client, error) } diff --git a/collector.go b/collector.go index 6d7cec3..55dd408 100644 --- a/collector.go +++ b/collector.go @@ -136,13 +136,23 @@ type CollectorFactory interface { Make(domain string, args CollectorFactoryArgs) (Collector, error) } +// CollectorFactoryWithDBProvider is an optional CollectorFactory capability +// for database-specific collectors that need access to the monitor-owned +// database provider. Factories that do not implement it retain the historical +// Make behavior. +type CollectorFactoryWithDBProvider interface { + CollectorFactory + MakeWithDBProvider(domain string, args CollectorFactoryArgs, provider DbProvider) (Collector, error) +} + // CollectorFactoryDatabaseTypes is an optional CollectorFactory capability // that identifies the database types supported by a domain. Factories that do // not implement this interface retain Blip's historical MySQL behavior. // // The domain argument allows one factory to serve domains with different // compatibility, such as MySQL collectors and database-neutral cloud metrics. -// Implementations must return at least one database type. +// Implementations must return at least one database type. Return +// DatabaseTypeAny by itself for a database-neutral domain. type CollectorFactoryDatabaseTypes interface { CollectorFactory DatabaseTypes(domain string) []DatabaseType diff --git a/config.go b/config.go index 1ac52de..7df483e 100644 --- a/config.go +++ b/config.go @@ -403,7 +403,8 @@ type ConfigMonitor struct { // Empty values retain Blip's historical MySQL behavior. DatabaseType DatabaseType `yaml:"database-type,omitempty"` - // ConfigMySQL: + // Shared connection identity and credentials. Socket and MyCnf are specific + // to Blip's built-in MySQL connection factory. Socket string `yaml:"socket,omitempty"` Hostname string `yaml:"hostname,omitempty"` MyCnf string `yaml:"mycnf,omitempty"` @@ -422,9 +423,12 @@ type ConfigMonitor struct { Heartbeat ConfigHeartbeat `yaml:"heartbeat,omitempty"` Plans ConfigPlans `yaml:"plans,omitempty"` Plan string `yaml:"plan,omitempty"` - Postgres ConfigPostgres `yaml:"postgres,omitempty"` - Sinks ConfigSinks `yaml:"sinks,omitempty"` - TLS ConfigTLS `yaml:"tls,omitempty"` + // DatabaseConfig is opaque configuration owned by the external module + // selected by DatabaseType. MySQL continues to use the historical monitor + // fields above and rejects this section. + DatabaseConfig ConfigDatabase `yaml:"database-config,omitempty"` + Sinks ConfigSinks `yaml:"sinks,omitempty"` + TLS ConfigTLS `yaml:"tls,omitempty"` Meta map[string]string `yaml:"meta,omitempty"` } @@ -434,13 +438,6 @@ const ( DEFAULT_MONITOR_TIMEOUT_CONNECT = "10s" ) -type DatabaseType string - -const ( - DatabaseTypeMySQL DatabaseType = "mysql" - DatabaseTypePostgres DatabaseType = "postgres" -) - func DefaultConfigMonitor() ConfigMonitor { return ConfigMonitor{ Username: DEFAULT_MONITOR_USERNAME, @@ -459,33 +456,55 @@ func DefaultConfigMonitor() ConfigMonitor { } // EffectiveDatabaseType returns the configured database type. An omitted -// value retains Blip's historical MySQL behavior. Environment interpolation is -// resolved here because monitor defaults are applied before the normal -// interpolation pass. +// value retains Blip's historical MySQL behavior. Direct environment-variable +// interpolation is resolved before defaults are selected; database type is a +// structural discriminator and does not support monitor-field interpolation. func (c ConfigMonitor) EffectiveDatabaseType() DatabaseType { - databaseType := DatabaseType(interpolateEnv(string(c.DatabaseType))) + databaseType := interpolateEnv(string(c.DatabaseType)) if databaseType == "" { return DatabaseTypeMySQL } - return databaseType + return DatabaseType(databaseType) } func (c ConfigMonitor) Validate() error { - switch c.EffectiveDatabaseType() { - case DatabaseTypeMySQL: - if c.Postgres.Set() { - return fmt.Errorf("config.monitor.postgres requires database-type %q", DatabaseTypePostgres) + databaseType := c.EffectiveDatabaseType() + if databaseType == DatabaseTypeMySQL { + if len(c.DatabaseConfig) > 0 { + return fmt.Errorf("config.monitor.database-config requires an external database type") } - case DatabaseTypePostgres: - if c.Socket != "" { - return fmt.Errorf("config.monitor.socket is only supported for database-type %q", DatabaseTypeMySQL) - } - if c.MyCnf != "" { - return fmt.Errorf("config.monitor.mycnf is only supported for database-type %q", DatabaseTypeMySQL) - } - return c.Postgres.Validate() - default: - return fmt.Errorf("config.monitor.database-type: invalid database type %q", c.DatabaseType) + return nil + } + if databaseType == DatabaseTypeAny { + return fmt.Errorf("config.monitor.database-type: %q is reserved for database-neutral collectors", databaseType) + } + if !ValidDatabaseType(databaseType) { + return fmt.Errorf("config.monitor.database-type: invalid database type %q", databaseType) + } + if c.Socket != "" { + return fmt.Errorf("config.monitor.socket is only supported for database-type %q", DatabaseTypeMySQL) + } + if c.MyCnf != "" { + return fmt.Errorf("config.monitor.mycnf is only supported for database-type %q", DatabaseTypeMySQL) + } + if c.Heartbeat.set() { + return fmt.Errorf("config.monitor.heartbeat is only supported for database-type %q", DatabaseTypeMySQL) + } + if c.Plans.Change.set() { + return fmt.Errorf("config.monitor.plans.change is only supported for database-type %q", DatabaseTypeMySQL) + } + if c.Plans.Table != "" { + return fmt.Errorf("config.monitor.plans.table is only supported for database-type %q", DatabaseTypeMySQL) + } + if c.Exporter.set() { + return fmt.Errorf("config.monitor.exporter is only supported for database-type %q", DatabaseTypeMySQL) + } + module, ok := registeredDatabaseModule(databaseType) + if !ok { + return fmt.Errorf("config.monitor.database-type: database module %q is not registered", databaseType) + } + if err := module.ValidateConfig(c); err != nil { + return fmt.Errorf("config.monitor.database-config for %q: %w", databaseType, err) } return nil } @@ -497,13 +516,17 @@ func (c ConfigMonitor) Redacted() ConfigMonitor { func (c ConfigMonitor) redacted(seen map[*ConfigMonitor]*ConfigMonitor) ConfigMonitor { c.Password = redactPassword(c.Password) + // External module configuration is opaque to Blip, so redact the entire + // section rather than guessing which module-owned fields might be secrets. + c.DatabaseConfig = nil c.Sinks = c.Sinks.redacted() c.Plans = c.Plans.redacted(seen) return c } func (c *ConfigMonitor) ApplyDefaults(b Config) { - if c.EffectiveDatabaseType() == DatabaseTypeMySQL { + databaseType := c.EffectiveDatabaseType() + if databaseType == DatabaseTypeMySQL { if c.Socket == "" { c.Socket = b.MySQL.Socket } @@ -545,15 +568,16 @@ func (c *ConfigMonitor) ApplyDefaults(b Config) { c.Sinks = ConfigSinks{} } c.AWS.ApplyDefaults(b) - c.Exporter.ApplyDefaults(b) c.HA.ApplyDefaults(b) - c.Heartbeat.ApplyDefaults(b) - c.Plans.ApplyDefaults(b) - if c.EffectiveDatabaseType() == DatabaseTypePostgres { - postgresDefaults := DefaultConfigPostgres() - postgresDefaults.ConnectTimeout = c.TimeoutConnect - c.Postgres.ApplyDefaults(postgresDefaults) - } + // Exporter emulation, heartbeat writes, and plan state changes are + // MySQL-specific. Do not inherit their global defaults into external database + // monitors; explicit monitor values remain intact so Validate can report the + // unsupported configuration. + if databaseType == DatabaseTypeMySQL { + c.Exporter.ApplyDefaults(b) + c.Heartbeat.ApplyDefaults(b) + } + c.Plans.applyDefaults(b, databaseType == DatabaseTypeMySQL) c.Sinks.ApplyDefaults(b) c.TLS.ApplyDefaults(b) } @@ -580,7 +604,7 @@ func (c *ConfigMonitor) InterpolateEnvVars() { c.Heartbeat.InterpolateEnvVars() c.Plans.InterpolateEnvVars() c.Plan = interpolateEnv(c.Plan) - c.Postgres.InterpolateEnvVars() + c.DatabaseConfig.interpolate(interpolateEnv) c.Sinks.InterpolateEnvVars() c.TLS.InterpolateEnvVars() } @@ -606,7 +630,7 @@ func (c *ConfigMonitor) InterpolateMonitor() { c.Heartbeat.InterpolateMonitor(c) c.Plans.InterpolateMonitor(c) c.Plan = c.interpolateMon(c.Plan) - c.Postgres.InterpolateMonitor(c) + c.DatabaseConfig.interpolate(c.interpolateMon) c.Sinks.InterpolateMonitor(c) c.TLS.InterpolateMonitor(c) } @@ -658,20 +682,6 @@ func (c *ConfigMonitor) fieldValue(f string) string { return c.PasswordFile case "timeout-connect": return c.TimeoutConnect - case "postgres.database": - return c.Postgres.Database - case "postgres.application-name": - return c.Postgres.ApplicationName - case "postgres.ssl-mode": - return c.Postgres.SSLMode - case "postgres.connect-timeout": - return c.Postgres.ConnectTimeout - case "postgres.statement-timeout": - return c.Postgres.StatementTimeout - case "postgres.lock-timeout": - return c.Postgres.LockTimeout - case "postgres.dial-address": - return c.Postgres.DialAddress default: return "" } @@ -735,6 +745,10 @@ type ConfigExporter struct { Plan string `yaml:"plan,omitempty"` } +func (c ConfigExporter) set() bool { + return c.Mode != "" || c.Plan != "" || len(c.Flags) > 0 +} + func DefaultConfigExporter() ConfigExporter { return ConfigExporter{} } @@ -798,6 +812,10 @@ type ConfigHeartbeat struct { Table string `yaml:"table,omitempty"` } +func (c ConfigHeartbeat) set() bool { + return c.Freq != "" || c.SourceId != "" || c.Role != "" || c.Table != "" +} + const ( DEFAULT_HEARTBEAT_TABLE = "blip.heartbeat" ) @@ -966,6 +984,15 @@ func DefaultConfigPlans() ConfigPlans { } func (c ConfigPlans) Validate() error { + if c.Table == "" || c.Monitor == nil { + return nil + } + if c.Monitor.EffectiveDatabaseType() != DatabaseTypeMySQL { + return fmt.Errorf("config.plans.monitor.database-type: config.plans.table is only supported for database-type %q", DatabaseTypeMySQL) + } + if err := c.Monitor.Validate(); err != nil { + return fmt.Errorf("config.plans.monitor: %w", err) + } return nil } @@ -986,11 +1013,17 @@ func (c ConfigPlans) redacted(seen map[*ConfigMonitor]*ConfigMonitor) ConfigPlan } func (c *ConfigPlans) ApplyDefaults(b Config) { + c.applyDefaults(b, true) +} + +func (c *ConfigPlans) applyDefaults(b Config, applyChange bool) { if len(c.Files) == 0 && len(b.Plans.Files) > 0 { c.Files = make([]string, len(b.Plans.Files)) copy(c.Files, b.Plans.Files) } - c.Change.ApplyDefaults(b) + if applyChange { + c.Change.ApplyDefaults(b) + } } func (c *ConfigPlans) InterpolateEnvVars() { @@ -1079,6 +1112,13 @@ func (c ConfigPlanChange) Enabled() bool { c.Active.Plan != "" } +func (c ConfigPlanChange) set() bool { + return c.Offline.After != "" || c.Offline.Plan != "" || + c.Standby.After != "" || c.Standby.Plan != "" || + c.ReadOnly.After != "" || c.ReadOnly.Plan != "" || + c.Active.After != "" || c.Active.Plan != "" +} + // -------------------------------------------------------------------------- type ConfigSinks map[string]map[string]string diff --git a/config_database_test.go b/config_database_test.go new file mode 100644 index 0000000..367f9cb --- /dev/null +++ b/config_database_test.go @@ -0,0 +1,536 @@ +// Copyright 2026 Block, Inc. + +package blip_test + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/cashapp/blip/v2" +) + +type testDatabaseModule struct { + databaseType blip.DatabaseType + validate func(blip.ConfigMonitor) error +} + +func (m testDatabaseModule) DatabaseType() blip.DatabaseType { + return m.databaseType +} + +func (m testDatabaseModule) ValidateConfig(cfg blip.ConfigMonitor) error { + if m.validate != nil { + return m.validate(cfg) + } + return nil +} + +func registerTestDatabaseModule(t *testing.T, databaseType blip.DatabaseType, validate func(blip.ConfigMonitor) error) { + t.Helper() + if err := blip.RegisterDatabaseModule(testDatabaseModule{databaseType: databaseType, validate: validate}); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { blip.RemoveDatabaseModule(databaseType) }) +} + +func TestConfigMonitorDatabaseTypeDefaultsToMySQLWithoutMutation(t *testing.T) { + monitor := blip.ConfigMonitor{} + monitor.ApplyDefaults(blip.DefaultConfig()) + + if monitor.DatabaseType != "" { + t.Fatalf("omitted database type mutated to %q", monitor.DatabaseType) + } + if monitor.EffectiveDatabaseType() != blip.DatabaseTypeMySQL { + t.Fatalf("effective database type = %q, expected mysql", monitor.EffectiveDatabaseType()) + } + if len(monitor.DatabaseConfig) != 0 { + t.Fatalf("external database config added to MySQL monitor: %#v", monitor.DatabaseConfig) + } +} + +func TestConfigMonitorExternalModuleValidationAndInterpolation(t *testing.T) { + const databaseType blip.DatabaseType = "test-interpolation" + t.Setenv("BLIP_TEST_DATABASE_TYPE", string(databaseType)) + t.Setenv("BLIP_TEST_DATABASE", "metrics_database") + t.Setenv("BLIP_TEST_INCLUDE", "app_*") + + type moduleConfig struct { + Database string `yaml:"database"` + Include []string `yaml:"include"` + ApplicationName string `yaml:"application-name"` + Nested struct { + Address string `yaml:"address"` + } `yaml:"nested"` + } + + var validated moduleConfig + registerTestDatabaseModule(t, databaseType, func(cfg blip.ConfigMonitor) error { + if cfg.TimeoutConnect != "7s" { + return errors.New("monitor defaults were not available to module validation") + } + return blip.DecodeDatabaseConfig(cfg.DatabaseConfig, &validated) + }) + + monitor := blip.ConfigMonitor{ + MonitorId: "external-monitor", + DatabaseType: "${BLIP_TEST_DATABASE_TYPE}", + TimeoutConnect: "7s", + DatabaseConfig: blip.ConfigDatabase{ + "database": "${BLIP_TEST_DATABASE}", + "include": []interface{}{"${BLIP_TEST_INCLUDE}"}, + "application-name": "%{monitor.id}", + "nested": map[interface{}]interface{}{ + "address": "%{monitor.hostname}", + }, + }, + Hostname: "database.example:1234", + } + monitor.ApplyDefaults(blip.DefaultConfig()) + monitor.InterpolateEnvVars() + monitor.InterpolateMonitor() + + if err := monitor.Validate(); err != nil { + t.Fatal(err) + } + if monitor.DatabaseType != databaseType { + t.Fatalf("database type = %q, expected %q", monitor.DatabaseType, databaseType) + } + if validated.Database != "metrics_database" { + t.Fatalf("database = %q", validated.Database) + } + if len(validated.Include) != 1 || validated.Include[0] != "app_*" { + t.Fatalf("include = %#v", validated.Include) + } + if validated.ApplicationName != monitor.MonitorId { + t.Fatalf("application name = %q", validated.ApplicationName) + } + if validated.Nested.Address != monitor.Hostname { + t.Fatalf("nested address = %q", validated.Nested.Address) + } +} + +func TestConfigMonitorExternalModuleInterpolationPreservesTypedContainers(t *testing.T) { + const databaseType blip.DatabaseType = "test-typed-interpolation" + t.Setenv("BLIP_TEST_TYPED_VALUE", "from-environment") + + type namedString string + type namedMap map[string]namedString + type namedSlice []namedString + type namedArray [2]namedString + type namedPointer *namedString + type namedConfig blip.ConfigDatabase + type nestedStruct struct { + Monitor namedString `yaml:"monitor"` + } + type namedStruct struct { + Environment namedString `yaml:"environment"` + Nested *nestedStruct `yaml:"nested"` + private namedString + } + type decodedStruct struct { + Environment string `yaml:"environment"` + Nested struct { + Monitor string `yaml:"monitor"` + } `yaml:"nested"` + } + + type moduleConfig struct { + NestedConfig map[string]string `yaml:"nested-config"` + TypedMap map[string]string `yaml:"typed-map"` + NamedMap map[string]string `yaml:"named-map"` + NamedSlice []string `yaml:"named-slice"` + NamedArray []string `yaml:"named-array"` + NamedStruct decodedStruct `yaml:"named-struct"` + NamedStructPointer decodedStruct `yaml:"named-struct-pointer"` + Pointer string `yaml:"pointer"` + NilMap map[string]string `yaml:"nil-map"` + NilSlice []string `yaml:"nil-slice"` + NilPointer *string `yaml:"nil-pointer"` + } + + var validated moduleConfig + registerTestDatabaseModule(t, databaseType, func(cfg blip.ConfigMonitor) error { + return blip.DecodeDatabaseConfig(cfg.DatabaseConfig, &validated) + }) + + pointer := namedString("%{monitor.id}") + structValue := namedStruct{ + Environment: "${BLIP_TEST_TYPED_VALUE}", + Nested: &nestedStruct{Monitor: "%{monitor.id}"}, + private: "${BLIP_TEST_TYPED_VALUE}", + } + monitor := blip.ConfigMonitor{ + MonitorId: "typed-monitor", + DatabaseType: databaseType, + DatabaseConfig: blip.ConfigDatabase{ + "nested-config": namedConfig{ + "environment": namedString("${BLIP_TEST_TYPED_VALUE}"), + }, + "typed-map": map[string]string{ + "monitor": "%{monitor.id}", + }, + "named-map": namedMap{ + "environment": "${BLIP_TEST_TYPED_VALUE}", + }, + "named-slice": namedSlice{"${BLIP_TEST_TYPED_VALUE}", "%{monitor.id}"}, + "named-array": namedArray{"${BLIP_TEST_TYPED_VALUE}", "%{monitor.id}"}, + "named-struct": structValue, + "named-struct-pointer": &structValue, + "pointer": namedPointer(&pointer), + "nil-map": map[string]string(nil), + "nil-slice": namedSlice(nil), + "nil-pointer": (*namedString)(nil), + }, + } + monitor.ApplyDefaults(blip.DefaultConfig()) + monitor.InterpolateEnvVars() + monitor.InterpolateMonitor() + + if err := monitor.Validate(); err != nil { + t.Fatal(err) + } + + nestedConfig, ok := monitor.DatabaseConfig["nested-config"].(namedConfig) + if !ok { + t.Fatalf("nested config type = %T, expected namedConfig", monitor.DatabaseConfig["nested-config"]) + } + if got := nestedConfig["environment"]; got != namedString("from-environment") { + t.Fatalf("nested environment = %q", got) + } + if _, ok := nestedConfig["environment"].(namedString); !ok { + t.Fatalf("nested environment type = %T, expected namedString", nestedConfig["environment"]) + } + + typedMap, ok := monitor.DatabaseConfig["typed-map"].(map[string]string) + if !ok || typedMap["monitor"] != monitor.MonitorId { + t.Fatalf("typed map = %#v (%T)", monitor.DatabaseConfig["typed-map"], monitor.DatabaseConfig["typed-map"]) + } + namedMapValue, ok := monitor.DatabaseConfig["named-map"].(namedMap) + if !ok || namedMapValue["environment"] != "from-environment" { + t.Fatalf("named map = %#v (%T)", monitor.DatabaseConfig["named-map"], monitor.DatabaseConfig["named-map"]) + } + namedSliceValue, ok := monitor.DatabaseConfig["named-slice"].(namedSlice) + if !ok || len(namedSliceValue) != 2 || namedSliceValue[0] != "from-environment" || namedSliceValue[1] != namedString(monitor.MonitorId) { + t.Fatalf("named slice = %#v (%T)", monitor.DatabaseConfig["named-slice"], monitor.DatabaseConfig["named-slice"]) + } + namedArrayValue, ok := monitor.DatabaseConfig["named-array"].(namedArray) + if !ok || namedArrayValue[0] != "from-environment" || namedArrayValue[1] != namedString(monitor.MonitorId) { + t.Fatalf("named array = %#v (%T)", monitor.DatabaseConfig["named-array"], monitor.DatabaseConfig["named-array"]) + } + namedStructValue, ok := monitor.DatabaseConfig["named-struct"].(namedStruct) + if !ok || namedStructValue.Environment != "from-environment" || namedStructValue.Nested == nil || namedStructValue.Nested.Monitor != namedString(monitor.MonitorId) { + t.Fatalf("named struct = %#v (%T)", monitor.DatabaseConfig["named-struct"], monitor.DatabaseConfig["named-struct"]) + } + if namedStructValue.private != "${BLIP_TEST_TYPED_VALUE}" { + t.Fatalf("unexported field was interpolated: %q", namedStructValue.private) + } + namedStructPointer, ok := monitor.DatabaseConfig["named-struct-pointer"].(*namedStruct) + if !ok || namedStructPointer == nil || namedStructPointer.Environment != "from-environment" || namedStructPointer.Nested == nil || namedStructPointer.Nested.Monitor != namedString(monitor.MonitorId) { + t.Fatalf("named struct pointer = %#v (%T)", monitor.DatabaseConfig["named-struct-pointer"], monitor.DatabaseConfig["named-struct-pointer"]) + } + if namedStructPointer.private != "${BLIP_TEST_TYPED_VALUE}" { + t.Fatalf("unexported pointer field was interpolated: %q", namedStructPointer.private) + } + pointerValue, ok := monitor.DatabaseConfig["pointer"].(namedPointer) + if !ok || pointerValue == nil || *pointerValue != namedString(monitor.MonitorId) { + t.Fatalf("pointer = %#v (%T)", monitor.DatabaseConfig["pointer"], monitor.DatabaseConfig["pointer"]) + } + if value, ok := monitor.DatabaseConfig["nil-map"].(map[string]string); !ok || value != nil { + t.Fatalf("nil map = %#v (%T)", monitor.DatabaseConfig["nil-map"], monitor.DatabaseConfig["nil-map"]) + } + if value, ok := monitor.DatabaseConfig["nil-slice"].(namedSlice); !ok || value != nil { + t.Fatalf("nil slice = %#v (%T)", monitor.DatabaseConfig["nil-slice"], monitor.DatabaseConfig["nil-slice"]) + } + if value, ok := monitor.DatabaseConfig["nil-pointer"].(*namedString); !ok || value != nil { + t.Fatalf("nil pointer = %#v (%T)", monitor.DatabaseConfig["nil-pointer"], monitor.DatabaseConfig["nil-pointer"]) + } + + if validated.NestedConfig["environment"] != "from-environment" || + validated.TypedMap["monitor"] != monitor.MonitorId || + validated.NamedMap["environment"] != "from-environment" || + len(validated.NamedSlice) != 2 || validated.NamedSlice[1] != monitor.MonitorId || + len(validated.NamedArray) != 2 || validated.NamedArray[1] != monitor.MonitorId || + validated.NamedStruct.Environment != "from-environment" || validated.NamedStruct.Nested.Monitor != monitor.MonitorId || + validated.NamedStructPointer.Environment != "from-environment" || validated.NamedStructPointer.Nested.Monitor != monitor.MonitorId || + validated.Pointer != monitor.MonitorId || + len(validated.NilMap) != 0 || len(validated.NilSlice) != 0 || validated.NilPointer != nil { + t.Fatalf("decoded module config = %#v", validated) + } +} + +func TestRegisterDatabaseModuleValidation(t *testing.T) { + tests := []struct { + name string + databaseType blip.DatabaseType + wantError string + }{ + {name: "empty", wantError: "invalid database type"}, + {name: "whitespace", databaseType: " test ", wantError: "invalid database type"}, + {name: "uppercase", databaseType: "Test", wantError: "invalid database type"}, + {name: "MySQL", databaseType: blip.DatabaseTypeMySQL, wantError: "built into Blip"}, + {name: "neutral marker", databaseType: blip.DatabaseTypeAny, wantError: "reserved"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := blip.RegisterDatabaseModule(testDatabaseModule{databaseType: tt.databaseType}) + if err == nil || !strings.Contains(err.Error(), tt.wantError) { + t.Fatalf("RegisterDatabaseModule error = %v", err) + } + }) + } + + const duplicate blip.DatabaseType = "test-duplicate" + registerTestDatabaseModule(t, duplicate, nil) + if err := blip.RegisterDatabaseModule(testDatabaseModule{databaseType: duplicate}); err == nil || !strings.Contains(err.Error(), "already registered") { + t.Fatalf("duplicate registration error = %v", err) + } + if err := blip.RegisterDatabaseModule(nil); err == nil || !strings.Contains(err.Error(), "nil") { + t.Fatalf("nil registration error = %v", err) + } + var nilModule *testDatabaseModule + if err := blip.RegisterDatabaseModule(nilModule); err == nil || !strings.Contains(err.Error(), "nil") { + t.Fatalf("typed nil registration error = %v", err) + } +} + +func TestConfigMonitorExternalDatabaseValidation(t *testing.T) { + const databaseType blip.DatabaseType = "test-guards" + registerTestDatabaseModule(t, databaseType, func(blip.ConfigMonitor) error { + return errors.New("module validation failed") + }) + + tests := []struct { + name string + monitor blip.ConfigMonitor + wantError string + }{ + { + name: "unregistered database type", + monitor: blip.ConfigMonitor{DatabaseType: "unregistered"}, + wantError: "is not registered", + }, + { + name: "database config on implicit MySQL monitor", + monitor: blip.ConfigMonitor{ + DatabaseConfig: blip.ConfigDatabase{"database": "example"}, + }, + wantError: "requires an external database type", + }, + { + name: "neutral marker as monitor type", + monitor: blip.ConfigMonitor{DatabaseType: blip.DatabaseTypeAny}, + wantError: "reserved", + }, + { + name: "monitor interpolation in database type", + monitor: blip.ConfigMonitor{DatabaseType: "%{monitor.meta.engine}", Meta: map[string]string{"engine": string(databaseType)}}, + wantError: "invalid database type", + }, + { + name: "my.cnf on external monitor", + monitor: blip.ConfigMonitor{DatabaseType: databaseType, MyCnf: "/etc/blip/my.cnf"}, + wantError: "mycnf is only supported", + }, + { + name: "socket on external monitor", + monitor: blip.ConfigMonitor{DatabaseType: databaseType, Socket: "/tmp/database.sock"}, + wantError: "socket is only supported", + }, + { + name: "heartbeat on external monitor", + monitor: blip.ConfigMonitor{DatabaseType: databaseType, Heartbeat: blip.ConfigHeartbeat{Freq: "1s"}}, + wantError: "heartbeat is only supported", + }, + { + name: "plan changing on external monitor", + monitor: blip.ConfigMonitor{DatabaseType: databaseType, Plans: blip.ConfigPlans{Change: blip.ConfigPlanChange{ + Active: blip.ConfigStatePlan{Plan: "active"}, + }}}, + wantError: "plans.change is only supported", + }, + { + name: "plan table on external monitor", + monitor: blip.ConfigMonitor{DatabaseType: databaseType, Plans: blip.ConfigPlans{Table: "blip.plans"}}, + wantError: "plans.table is only supported", + }, + { + name: "exporter on external monitor", + monitor: blip.ConfigMonitor{DatabaseType: databaseType, Exporter: blip.ConfigExporter{Mode: blip.EXPORTER_MODE_DUAL, Plan: "external"}}, + wantError: "exporter is only supported", + }, + { + name: "module validation", + monitor: blip.ConfigMonitor{DatabaseType: databaseType}, + wantError: "module validation failed", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + monitor := tt.monitor + monitor.InterpolateEnvVars() + monitor.InterpolateMonitor() + err := monitor.Validate() + if err == nil || !strings.Contains(err.Error(), tt.wantError) { + t.Fatalf("got error %v, expected it to contain %q", err, tt.wantError) + } + }) + } +} + +func TestConfigMonitorDatabaseSpecificDefaults(t *testing.T) { + const databaseType blip.DatabaseType = "test-defaults" + registerTestDatabaseModule(t, databaseType, nil) + + defaults := blip.DefaultConfig() + defaults.MySQL.Hostname = "mysql.example:3306" + defaults.Exporter = blip.ConfigExporter{Mode: blip.EXPORTER_MODE_DUAL} + defaults.Heartbeat = blip.ConfigHeartbeat{Freq: "1s", Table: "blip.heartbeat"} + defaults.Plans = blip.ConfigPlans{ + Files: []string{"shared.yaml"}, + Change: blip.ConfigPlanChange{Active: blip.ConfigStatePlan{Plan: "active"}}, + } + + external := blip.ConfigMonitor{DatabaseType: databaseType} + external.ApplyDefaults(defaults) + if external.Hostname != "" { + t.Fatalf("external monitor inherited MySQL hostname %q", external.Hostname) + } + if external.Exporter.Mode != "" || external.Exporter.Plan != "" || len(external.Exporter.Flags) != 0 { + t.Fatalf("external monitor inherited exporter defaults: %+v", external.Exporter) + } + if external.Heartbeat != (blip.ConfigHeartbeat{}) { + t.Fatalf("external monitor inherited heartbeat defaults: %+v", external.Heartbeat) + } + if external.Plans.Change.Enabled() { + t.Fatalf("external monitor inherited plan-changing defaults: %+v", external.Plans.Change) + } + if got := external.Plans.Files; len(got) != 1 || got[0] != "shared.yaml" { + t.Fatalf("external monitor plan files = %#v", got) + } + if err := external.Validate(); err != nil { + t.Fatalf("external monitor with shared defaults is invalid: %v", err) + } + + mysql := blip.ConfigMonitor{} + mysql.ApplyDefaults(defaults) + if mysql.Hostname != defaults.MySQL.Hostname { + t.Fatalf("MySQL hostname = %q", mysql.Hostname) + } + if mysql.Exporter.Plan != blip.DEFAULT_EXPORTER_PLAN { + t.Fatalf("MySQL exporter plan = %q", mysql.Exporter.Plan) + } + if mysql.Heartbeat == (blip.ConfigHeartbeat{}) || !mysql.Plans.Change.Enabled() { + t.Fatal("MySQL monitor did not inherit MySQL-specific defaults") + } +} + +func TestConfigMonitorEnvironmentTypeSelectsDefaultsBeforeInterpolation(t *testing.T) { + const databaseType blip.DatabaseType = "test-env-defaults" + registerTestDatabaseModule(t, databaseType, nil) + t.Setenv("BLIP_TEST_ENV_DATABASE_TYPE", string(databaseType)) + + defaults := blip.DefaultConfig() + defaults.MySQL.Hostname = "mysql.example:3306" + monitor := blip.ConfigMonitor{DatabaseType: "${BLIP_TEST_ENV_DATABASE_TYPE}"} + monitor.ApplyDefaults(defaults) + if monitor.Hostname != "" { + t.Fatalf("external monitor inherited MySQL hostname %q", monitor.Hostname) + } + monitor.InterpolateEnvVars() + monitor.InterpolateMonitor() + if monitor.DatabaseType != databaseType { + t.Fatalf("database type = %q", monitor.DatabaseType) + } + if err := monitor.Validate(); err != nil { + t.Fatal(err) + } +} + +func TestConfigPlansRejectsExternalTableMonitor(t *testing.T) { + plans := blip.ConfigPlans{ + Table: "blip.plans", + Monitor: &blip.ConfigMonitor{DatabaseType: "external-without-registration"}, + } + err := plans.Validate() + if err == nil || !strings.Contains(err.Error(), "config.plans.table is only supported") { + t.Fatalf("ConfigPlans.Validate error = %v", err) + } +} + +func TestConfigMonitorRedactsOpaqueDatabaseConfig(t *testing.T) { + monitor := blip.ConfigMonitor{ + DatabaseType: "external", + DatabaseConfig: blip.ConfigDatabase{ + "password": "do-not-log", + "nested": map[string]interface{}{"token": "also-do-not-log"}, + }, + } + redacted := monitor.Redacted() + if redacted.DatabaseConfig != nil { + t.Fatalf("redacted database config = %#v", redacted.DatabaseConfig) + } + if monitor.DatabaseConfig["password"] != "do-not-log" { + t.Fatal("redaction modified live database config") + } +} + +func TestDecodeDatabaseConfigIsStrict(t *testing.T) { + type moduleConfig struct { + Database string `yaml:"database"` + } + + var decoded moduleConfig + if err := blip.DecodeDatabaseConfig(blip.ConfigDatabase{"database": "metrics"}, &decoded); err != nil { + t.Fatal(err) + } + if decoded.Database != "metrics" { + t.Fatalf("decoded database = %q", decoded.Database) + } + if err := blip.DecodeDatabaseConfig(blip.ConfigDatabase{"unknown": true}, &decoded); err == nil || !strings.Contains(err.Error(), "field unknown not found") { + t.Fatalf("strict decode error = %v", err) + } + if err := blip.DecodeDatabaseConfig(nil, nil); err == nil { + t.Fatal("nil decode output succeeded") + } + var nilOutput *moduleConfig + if err := blip.DecodeDatabaseConfig(nil, nilOutput); err == nil { + t.Fatal("typed nil decode output succeeded") + } +} + +func TestLoadConfigAcceptsOpaqueModuleShapeForStrictModuleDecode(t *testing.T) { + const databaseType blip.DatabaseType = "test-yaml" + registerTestDatabaseModule(t, databaseType, func(cfg blip.ConfigMonitor) error { + var decoded struct { + Database string `yaml:"database"` + } + return blip.DecodeDatabaseConfig(cfg.DatabaseConfig, &decoded) + }) + + configFile := filepath.Join(t.TempDir(), "blip.yaml") + contents := `monitors: + - id: external + database-type: test-yaml + database-config: + database: metrics + unknown: rejected-by-module +` + if err := os.WriteFile(configFile, []byte(contents), 0o600); err != nil { + t.Fatal(err) + } + cfg, err := blip.LoadConfig(configFile, blip.DefaultConfig(), true) + if err != nil { + t.Fatalf("Blip strict YAML decode rejected opaque module config: %v", err) + } + monitor := cfg.Monitors[0] + monitor.ApplyDefaults(cfg) + monitor.InterpolateEnvVars() + monitor.InterpolateMonitor() + if err := monitor.Validate(); err == nil || !strings.Contains(err.Error(), "field unknown not found") { + t.Fatalf("module strict validation error = %v", err) + } +} diff --git a/config_postgres.go b/config_postgres.go deleted file mode 100644 index fd13a06..0000000 --- a/config_postgres.go +++ /dev/null @@ -1,187 +0,0 @@ -// Copyright 2026 Block, Inc. - -package blip - -import ( - "fmt" - "strings" - "time" -) - -const ( - DEFAULT_POSTGRES_DATABASE = "postgres" - DEFAULT_POSTGRES_APPLICATION_NAME = "pgblip" - DEFAULT_POSTGRES_MAX_OPEN_CONNECTIONS = 4 - DEFAULT_POSTGRES_MAX_IDLE_CONNECTIONS = 2 - DEFAULT_POSTGRES_MAX_CONNECTION_IDLE_TIME = "30s" - DEFAULT_POSTGRES_MAX_CONNECTION_LIFETIME = "0" -) - -// ConfigPostgres configures the PostgreSQL database/sql pool owned by one -// monitor. Credentials and TLS certificate files remain in the existing -// monitor-level fields so all Blip credential sources can be shared by -// database-specific connection factories. -type ConfigPostgres struct { - Database string `yaml:"database,omitempty"` - ApplicationName string `yaml:"application-name,omitempty"` - SSLMode string `yaml:"ssl-mode,omitempty"` - MaxOpenConnections *int `yaml:"max-open-connections,omitempty"` - MaxIdleConnections *int `yaml:"max-idle-connections,omitempty"` - MaxConnectionIdleTime string `yaml:"max-connection-idle-time,omitempty"` - MaxConnectionLifetime string `yaml:"max-connection-lifetime,omitempty"` - ConnectTimeout string `yaml:"connect-timeout,omitempty"` - StatementTimeout string `yaml:"statement-timeout,omitempty"` - LockTimeout string `yaml:"lock-timeout,omitempty"` - DialAddress string `yaml:"dial-address,omitempty"` -} - -func DefaultConfigPostgres() ConfigPostgres { - return ConfigPostgres{ - Database: DEFAULT_POSTGRES_DATABASE, - ApplicationName: DEFAULT_POSTGRES_APPLICATION_NAME, - MaxOpenConnections: postgresInt(DEFAULT_POSTGRES_MAX_OPEN_CONNECTIONS), - MaxIdleConnections: postgresInt(DEFAULT_POSTGRES_MAX_IDLE_CONNECTIONS), - MaxConnectionIdleTime: DEFAULT_POSTGRES_MAX_CONNECTION_IDLE_TIME, - MaxConnectionLifetime: DEFAULT_POSTGRES_MAX_CONNECTION_LIFETIME, - ConnectTimeout: DEFAULT_MONITOR_TIMEOUT_CONNECT, - } -} - -// Set reports whether a monitor explicitly contains PostgreSQL configuration. -func (c ConfigPostgres) Set() bool { - return c.Database != "" || - c.ApplicationName != "" || - c.SSLMode != "" || - c.MaxOpenConnections != nil || - c.MaxIdleConnections != nil || - c.MaxConnectionIdleTime != "" || - c.MaxConnectionLifetime != "" || - c.ConnectTimeout != "" || - c.StatementTimeout != "" || - c.LockTimeout != "" || - c.DialAddress != "" -} - -func (c *ConfigPostgres) ApplyDefaults(defaults ConfigPostgres) { - if c.Database == "" { - c.Database = defaults.Database - } - if c.ApplicationName == "" { - c.ApplicationName = defaults.ApplicationName - } - if c.SSLMode == "" { - c.SSLMode = defaults.SSLMode - } - c.MaxOpenConnections = setPostgresInt(c.MaxOpenConnections, defaults.MaxOpenConnections) - c.MaxIdleConnections = setPostgresInt(c.MaxIdleConnections, defaults.MaxIdleConnections) - if c.MaxConnectionIdleTime == "" { - c.MaxConnectionIdleTime = defaults.MaxConnectionIdleTime - } - if c.MaxConnectionLifetime == "" { - c.MaxConnectionLifetime = defaults.MaxConnectionLifetime - } - if c.ConnectTimeout == "" { - c.ConnectTimeout = defaults.ConnectTimeout - } - if c.StatementTimeout == "" { - c.StatementTimeout = defaults.StatementTimeout - } - if c.LockTimeout == "" { - c.LockTimeout = defaults.LockTimeout - } - if c.DialAddress == "" { - c.DialAddress = defaults.DialAddress - } -} - -func (c ConfigPostgres) Validate() error { - validSSLModes := map[string]bool{ - "": true, - "disable": true, - "allow": true, - "prefer": true, - "require": true, - "verify-ca": true, - "verify-full": true, - } - if !validSSLModes[strings.ToLower(c.SSLMode)] { - return fmt.Errorf("config.postgres.ssl-mode: invalid PostgreSQL SSL mode %q", c.SSLMode) - } - if c.MaxOpenConnections != nil && *c.MaxOpenConnections < 0 { - return fmt.Errorf("config.postgres.max-open-connections: must be greater than or equal to zero") - } - if c.MaxIdleConnections != nil && *c.MaxIdleConnections < 0 { - return fmt.Errorf("config.postgres.max-idle-connections: must be greater than or equal to zero") - } - if c.MaxOpenConnections != nil && c.MaxIdleConnections != nil && - *c.MaxOpenConnections > 0 && *c.MaxIdleConnections > *c.MaxOpenConnections { - return fmt.Errorf("config.postgres.max-idle-connections: cannot exceed max-open-connections") - } - if err := validatePostgresDuration("connect-timeout", c.ConnectTimeout, false); err != nil { - return err - } - if err := validatePostgresDuration("max-connection-idle-time", c.MaxConnectionIdleTime, true); err != nil { - return err - } - if err := validatePostgresDuration("max-connection-lifetime", c.MaxConnectionLifetime, true); err != nil { - return err - } - if err := validatePostgresDuration("statement-timeout", c.StatementTimeout, true); err != nil { - return err - } - return validatePostgresDuration("lock-timeout", c.LockTimeout, true) -} - -func postgresInt(value int) *int { - return &value -} - -func setPostgresInt(value, defaultValue *int) *int { - if value != nil || defaultValue == nil { - return value - } - copy := *defaultValue - return © -} - -func validatePostgresDuration(name, value string, allowZero bool) error { - if value == "" { - return nil - } - duration, err := time.ParseDuration(value) - if err != nil { - return fmt.Errorf("config.postgres.%s: invalid duration %q: %w", name, value, err) - } - if duration < 0 || (!allowZero && duration == 0) { - constraint := "greater than zero" - if allowZero { - constraint = "greater than or equal to zero" - } - return fmt.Errorf("config.postgres.%s: must be %s", name, constraint) - } - return nil -} - -func (c *ConfigPostgres) InterpolateEnvVars() { - c.Database = interpolateEnv(c.Database) - c.ApplicationName = interpolateEnv(c.ApplicationName) - c.SSLMode = interpolateEnv(c.SSLMode) - c.MaxConnectionIdleTime = interpolateEnv(c.MaxConnectionIdleTime) - c.MaxConnectionLifetime = interpolateEnv(c.MaxConnectionLifetime) - c.ConnectTimeout = interpolateEnv(c.ConnectTimeout) - c.StatementTimeout = interpolateEnv(c.StatementTimeout) - c.LockTimeout = interpolateEnv(c.LockTimeout) - c.DialAddress = interpolateEnv(c.DialAddress) -} - -func (c *ConfigPostgres) InterpolateMonitor(m *ConfigMonitor) { - c.Database = m.interpolateMon(c.Database) - c.ApplicationName = m.interpolateMon(c.ApplicationName) - c.SSLMode = m.interpolateMon(c.SSLMode) - c.MaxConnectionIdleTime = m.interpolateMon(c.MaxConnectionIdleTime) - c.MaxConnectionLifetime = m.interpolateMon(c.MaxConnectionLifetime) - c.ConnectTimeout = m.interpolateMon(c.ConnectTimeout) - c.StatementTimeout = m.interpolateMon(c.StatementTimeout) - c.LockTimeout = m.interpolateMon(c.LockTimeout) - c.DialAddress = m.interpolateMon(c.DialAddress) -} diff --git a/config_postgres_test.go b/config_postgres_test.go deleted file mode 100644 index 8c3ae43..0000000 --- a/config_postgres_test.go +++ /dev/null @@ -1,179 +0,0 @@ -// Copyright 2026 Block, Inc. - -package blip_test - -import ( - "strings" - "testing" - - "github.com/cashapp/blip/v2" -) - -func TestConfigMonitorDatabaseTypeDefaultsToMySQLWithoutMutation(t *testing.T) { - monitor := blip.ConfigMonitor{} - monitor.ApplyDefaults(blip.DefaultConfig()) - - if monitor.DatabaseType != "" { - t.Fatalf("omitted database type mutated to %q", monitor.DatabaseType) - } - if monitor.EffectiveDatabaseType() != blip.DatabaseTypeMySQL { - t.Fatalf("effective database type = %q, expected mysql", monitor.EffectiveDatabaseType()) - } - if monitor.Postgres.Set() { - t.Fatalf("PostgreSQL defaults added to MySQL monitor: %+v", monitor.Postgres) - } -} - -func TestConfigMonitorPostgresDefaultsAndInterpolation(t *testing.T) { - t.Setenv("BLIP_TEST_DATABASE_TYPE", "postgres") - t.Setenv("BLIP_TEST_POSTGRES_DATABASE", "metrics_database") - t.Setenv("BLIP_TEST_POSTGRES_DIAL_ADDRESS", "127.0.0.1:35432") - - monitor := blip.ConfigMonitor{ - MonitorId: "postgres-monitor", - DatabaseType: "${BLIP_TEST_DATABASE_TYPE}", - TimeoutConnect: "7s", - Postgres: blip.ConfigPostgres{ - Database: "${BLIP_TEST_POSTGRES_DATABASE}", - ApplicationName: "%{monitor.id}", - DialAddress: "${BLIP_TEST_POSTGRES_DIAL_ADDRESS}", - }, - } - monitor.ApplyDefaults(blip.DefaultConfig()) - monitor.InterpolateEnvVars() - monitor.InterpolateMonitor() - - if err := monitor.Validate(); err != nil { - t.Fatal(err) - } - if monitor.DatabaseType != blip.DatabaseTypePostgres { - t.Fatalf("database type = %q, expected postgres", monitor.DatabaseType) - } - if monitor.Postgres.Database != "metrics_database" { - t.Fatalf("database = %q, expected metrics_database", monitor.Postgres.Database) - } - if monitor.Postgres.ApplicationName != monitor.MonitorId { - t.Fatalf("application name = %q, expected monitor ID %q", monitor.Postgres.ApplicationName, monitor.MonitorId) - } - if monitor.Postgres.DialAddress != "127.0.0.1:35432" { - t.Fatalf("dial address = %q, expected interpolated address", monitor.Postgres.DialAddress) - } - if monitor.Postgres.ConnectTimeout != "7s" { - t.Fatalf("connect timeout = %q, expected inherited monitor timeout", monitor.Postgres.ConnectTimeout) - } - if monitor.Postgres.MaxOpenConnections == nil || *monitor.Postgres.MaxOpenConnections != blip.DEFAULT_POSTGRES_MAX_OPEN_CONNECTIONS { - t.Fatalf("max open connections not defaulted: %+v", monitor.Postgres.MaxOpenConnections) - } - if monitor.Postgres.MaxIdleConnections == nil || *monitor.Postgres.MaxIdleConnections != blip.DEFAULT_POSTGRES_MAX_IDLE_CONNECTIONS { - t.Fatalf("max idle connections not defaulted: %+v", monitor.Postgres.MaxIdleConnections) - } -} - -func TestConfigMonitorDatabaseTypeValidation(t *testing.T) { - tests := []struct { - name string - monitor blip.ConfigMonitor - wantError string - }{ - { - name: "unknown database type", - monitor: blip.ConfigMonitor{DatabaseType: "oracle"}, - wantError: "invalid database type", - }, - { - name: "PostgreSQL config on implicit MySQL monitor", - monitor: blip.ConfigMonitor{ - Postgres: blip.ConfigPostgres{Database: "postgres"}, - }, - wantError: "requires database-type", - }, - { - name: "my.cnf on PostgreSQL monitor", - monitor: blip.ConfigMonitor{ - DatabaseType: blip.DatabaseTypePostgres, - MyCnf: "/etc/blip/my.cnf", - }, - wantError: "mycnf is only supported", - }, - { - name: "socket on PostgreSQL monitor", - monitor: blip.ConfigMonitor{ - DatabaseType: blip.DatabaseTypePostgres, - Socket: "/tmp/.s.PGSQL.5432", - }, - wantError: "socket is only supported", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := tt.monitor.Validate() - if err == nil || !strings.Contains(err.Error(), tt.wantError) { - t.Fatalf("got error %v, expected it to contain %q", err, tt.wantError) - } - }) - } -} - -func TestConfigPostgresAllowsExplicitUnlimitedPoolSettings(t *testing.T) { - zero := 0 - config := blip.ConfigPostgres{ - Database: "postgres", - MaxOpenConnections: &zero, - MaxIdleConnections: &zero, - } - config.ApplyDefaults(blip.DefaultConfigPostgres()) - - if *config.MaxOpenConnections != 0 || *config.MaxIdleConnections != 0 { - t.Fatalf("explicit zero pool settings were overwritten: %+v", config) - } -} - -func TestConfigPostgresValidation(t *testing.T) { - minusOne := -1 - one := 1 - two := 2 - tests := []struct { - name string - config blip.ConfigPostgres - wantError string - }{ - { - name: "invalid SSL mode", - config: blip.ConfigPostgres{SSLMode: "invalid"}, - wantError: "ssl-mode", - }, - { - name: "negative max open", - config: blip.ConfigPostgres{MaxOpenConnections: &minusOne}, - wantError: "max-open-connections", - }, - { - name: "idle exceeds open", - config: blip.ConfigPostgres{ - MaxOpenConnections: &one, - MaxIdleConnections: &two, - }, - wantError: "cannot exceed", - }, - { - name: "zero connect timeout", - config: blip.ConfigPostgres{ConnectTimeout: "0"}, - wantError: "connect-timeout", - }, - { - name: "invalid lifetime", - config: blip.ConfigPostgres{MaxConnectionLifetime: "tomorrow"}, - wantError: "max-connection-lifetime", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := tt.config.Validate() - if err == nil || !strings.Contains(err.Error(), tt.wantError) { - t.Fatalf("got error %v, expected it to contain %q", err, tt.wantError) - } - }) - } -} diff --git a/credentials/credentials.go b/credentials/credentials.go index f6f734a..bed3f18 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -36,8 +36,9 @@ func NewFactory(awsConfig blip.AWSConfigFactory, passwordSecretParser blip.Passw // Dynamic returns the first configured shared reloadable source in Blip's // established precedence order: IAM, Secrets Manager, then password file. The // boolean reports whether a source was selected. Engine-specific factories can -// insert their own sources before falling back to Static. -func (f Factory) Dynamic(cfg blip.ConfigMonitor) (Func, bool, error) { +// insert their own sources before falling back to Static. defaultPort is used +// only when signing an IAM token for a hostname without an explicit port. +func (f Factory) Dynamic(cfg blip.ConfigMonitor, defaultPort string) (Func, bool, error) { if blip.True(cfg.AWS.IAMAuth) { blip.Debug("%s: AWS IAM auth token password", cfg.MonitorId) if f.awsConfig == nil { @@ -47,7 +48,10 @@ func (f Factory) Dynamic(cfg blip.ConfigMonitor) (Func, bool, error) { if err != nil { return nil, true, err } - token := blipaws.NewAuthToken(cfg.Username, cfg.Hostname, awscfg) + if defaultPort == "" { + return nil, true, fmt.Errorf("AWS IAM authentication requires a database default port") + } + token := blipaws.NewAuthTokenWithDefaultPort(cfg.Username, cfg.Hostname, defaultPort, awscfg) return func(ctx context.Context) (blip.DbCredentials, error) { password, err := token.Password(ctx) if err != nil { diff --git a/credentials/credentials_test.go b/credentials/credentials_test.go index 755fd85..493f7e9 100644 --- a/credentials/credentials_test.go +++ b/credentials/credentials_test.go @@ -4,15 +4,26 @@ package credentials_test import ( "context" + "net/url" "os" "path/filepath" "strings" "testing" + awsv2 "github.com/aws/aws-sdk-go-v2/aws" + "github.com/cashapp/blip/v2" "github.com/cashapp/blip/v2/credentials" ) +type staticAWSConfigFactory struct { + config awsv2.Config +} + +func (f staticAWSConfigFactory) Make(blip.AWS, string) (awsv2.Config, error) { + return f.config, nil +} + func TestDynamicPasswordFileReloads(t *testing.T) { passwordFile := filepath.Join(t.TempDir(), "password") if err := os.WriteFile(passwordFile, []byte("first"), 0o600); err != nil { @@ -22,7 +33,7 @@ func TestDynamicPasswordFileReloads(t *testing.T) { credentialFunc, selected, err := credentials.NewFactory(nil, nil).Dynamic(blip.ConfigMonitor{ Username: "metrics", PasswordFile: passwordFile, - }) + }, "3306") if err != nil { t.Fatal(err) } @@ -79,7 +90,7 @@ func TestDynamicPreservesSourcePrecedence(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - credentialFunc, selected, err := credentials.NewFactory(nil, nil).Dynamic(tt.config) + credentialFunc, selected, err := credentials.NewFactory(nil, nil).Dynamic(tt.config, "3306") if !selected { t.Fatal("configured source was not selected") } @@ -93,11 +104,98 @@ func TestDynamicPreservesSourcePrecedence(t *testing.T) { } } +func TestDynamicIAMUsesCallerDefaultPort(t *testing.T) { + iamAuth := true + factory := credentials.NewFactory(staticAWSConfigFactory{config: awsv2.Config{ + Region: "us-west-2", + Credentials: awsv2.CredentialsProviderFunc(func(context.Context) (awsv2.Credentials, error) { + return awsv2.Credentials{ + AccessKeyID: "test-access-key-do-not-use", + SecretAccessKey: "test-secret-key-do-not-use", + SessionToken: "test-session-token-do-not-use", + }, nil + }), + }}, nil) + + tests := []struct { + name string + defaultPort string + hostname string + wantHost string + }{ + { + name: "MySQL default", + defaultPort: "3306", + hostname: "mysql.example", + wantHost: "mysql.example:3306", + }, + { + name: "external engine default", + defaultPort: "5432", + hostname: "postgres.example", + wantHost: "postgres.example:5432", + }, + { + name: "explicit port", + defaultPort: "5432", + hostname: "postgres.example:6432", + wantHost: "postgres.example:6432", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + credentialFunc, selected, err := factory.Dynamic(blip.ConfigMonitor{ + Hostname: tt.hostname, + Username: "metrics", + AWS: blip.ConfigAWS{ + IAMAuth: &iamAuth, + Region: "us-west-2", + }, + }, tt.defaultPort) + if err != nil { + t.Fatal(err) + } + if !selected { + t.Fatal("IAM credentials were not selected") + } + + got, err := credentialFunc(context.Background()) + if err != nil { + t.Fatal(err) + } + tokenURL, err := url.Parse("https://" + got.Password) + if err != nil { + t.Fatal(err) + } + if tokenURL.Host != tt.wantHost { + t.Fatalf("IAM token host = %q, expected %q", tokenURL.Host, tt.wantHost) + } + }) + } +} + +func TestDynamicIAMRequiresCallerDefaultPort(t *testing.T) { + iamAuth := true + factory := credentials.NewFactory(staticAWSConfigFactory{config: awsv2.Config{Region: "us-west-2"}}, nil) + credentialFunc, selected, err := factory.Dynamic(blip.ConfigMonitor{ + Hostname: "database.example", + Username: "metrics", + AWS: blip.ConfigAWS{ + IAMAuth: &iamAuth, + Region: "us-west-2", + }, + }, "") + if !selected || credentialFunc != nil || err == nil || !strings.Contains(err.Error(), "default port") { + t.Fatalf("Dynamic returned func=%v selected=%t err=%v", credentialFunc != nil, selected, err) + } +} + func TestDynamicReportsNoSharedSource(t *testing.T) { credentialFunc, selected, err := credentials.NewFactory(nil, nil).Dynamic(blip.ConfigMonitor{ Username: "metrics", Password: "static", - }) + }, "3306") if err != nil || selected || credentialFunc != nil { t.Fatalf("Dynamic returned func=%v selected=%t err=%v", credentialFunc != nil, selected, err) } diff --git a/database_module.go b/database_module.go new file mode 100644 index 0000000..c7dbaa6 --- /dev/null +++ b/database_module.go @@ -0,0 +1,210 @@ +// Copyright 2026 Block, Inc. + +package blip + +import ( + "fmt" + "reflect" + "regexp" + "sync" + + "gopkg.in/yaml.v2" +) + +// DatabaseType identifies the database engine used by one monitor. +type DatabaseType string + +const ( + // DatabaseTypeMySQL is Blip's built-in database type. An omitted monitor + // database type retains this historical behavior. + DatabaseTypeMySQL DatabaseType = "mysql" + + // DatabaseTypeAny declares that a collector is database-neutral. It is a + // collector compatibility marker, not a valid monitor database type. + DatabaseTypeAny DatabaseType = "*" +) + +// ConfigDatabase contains configuration owned by an external database module. +// Blip interpolates string values but otherwise treats this map as opaque. A +// module should use DecodeDatabaseConfig to strictly decode it into a typed +// configuration before applying its defaults and validation. +type ConfigDatabase map[string]interface{} + +// DatabaseModule validates configuration for one external database type. +// Modules register before server boot. MySQL is built into Blip and does not +// use this interface. +type DatabaseModule interface { + DatabaseType() DatabaseType + ValidateConfig(ConfigMonitor) error +} + +var databaseModuleRegistry = struct { + sync.RWMutex + modules map[DatabaseType]DatabaseModule +}{ + modules: map[DatabaseType]DatabaseModule{}, +} + +var validDatabaseType = regexp.MustCompile(`^[a-z][a-z0-9._-]*$`) + +// ValidDatabaseType reports whether a value is a valid concrete monitor and +// collector database type. DatabaseTypeAny is not a concrete type. +func ValidDatabaseType(databaseType DatabaseType) bool { + return validDatabaseType.MatchString(string(databaseType)) +} + +// RegisterDatabaseModule registers one external database module. Registering a +// duplicate type or one of Blip's reserved database types returns an error. +func RegisterDatabaseModule(module DatabaseModule) error { + if nilInterface(module) { + return fmt.Errorf("database module is nil") + } + databaseType := module.DatabaseType() + if databaseType == DatabaseTypeMySQL { + return fmt.Errorf("database type %q is built into Blip", databaseType) + } + if databaseType == DatabaseTypeAny { + return fmt.Errorf("database type %q is reserved for database-neutral collectors", databaseType) + } + if !ValidDatabaseType(databaseType) { + return fmt.Errorf("database module has invalid database type %q", databaseType) + } + + databaseModuleRegistry.Lock() + defer databaseModuleRegistry.Unlock() + if _, ok := databaseModuleRegistry.modules[databaseType]; ok { + return fmt.Errorf("database module %q already registered", databaseType) + } + databaseModuleRegistry.modules[databaseType] = module + return nil +} + +// RemoveDatabaseModule removes an external database module. It supports test +// isolation and registration rollback when a larger module enable operation +// fails after registering its database type. +func RemoveDatabaseModule(databaseType DatabaseType) { + databaseModuleRegistry.Lock() + defer databaseModuleRegistry.Unlock() + delete(databaseModuleRegistry.modules, databaseType) +} + +func registeredDatabaseModule(databaseType DatabaseType) (DatabaseModule, bool) { + databaseModuleRegistry.RLock() + defer databaseModuleRegistry.RUnlock() + module, ok := databaseModuleRegistry.modules[databaseType] + return module, ok +} + +// DecodeDatabaseConfig strictly decodes opaque monitor database configuration +// into a module-owned typed value. +func DecodeDatabaseConfig(config ConfigDatabase, out interface{}) error { + if nilInterface(out) { + return fmt.Errorf("database config output is nil") + } + encoded, err := yaml.Marshal(config) + if err != nil { + return fmt.Errorf("encode database config: %w", err) + } + if err := yaml.UnmarshalStrict(encoded, out); err != nil { + return fmt.Errorf("decode database config: %w", err) + } + return nil +} + +func nilInterface(value interface{}) bool { + if value == nil { + return true + } + reflected := reflect.ValueOf(value) + switch reflected.Kind() { + case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Ptr, reflect.Slice: + return reflected.IsNil() + default: + return false + } +} + +func (c ConfigDatabase) interpolate(interpolate func(string) string) { + for key, value := range c { + c[key] = interpolateDatabaseConfigValue(value, interpolate) + } +} + +func interpolateDatabaseConfigValue(value interface{}, interpolate func(string) string) interface{} { + interpolated := interpolateDatabaseConfigReflect(reflect.ValueOf(value), interpolate) + if !interpolated.IsValid() { + return nil + } + return interpolated.Interface() +} + +func interpolateDatabaseConfigReflect(value reflect.Value, interpolate func(string) string) reflect.Value { + if !value.IsValid() { + return value + } + + switch value.Kind() { + case reflect.Interface: + if value.IsNil() { + return reflect.Zero(value.Type()) + } + interpolated := interpolateDatabaseConfigReflect(value.Elem(), interpolate) + result := reflect.New(value.Type()).Elem() + result.Set(interpolated) + return result + case reflect.String: + result := reflect.New(value.Type()).Elem() + result.SetString(interpolate(value.String())) + return result + case reflect.Map: + if value.IsNil() { + return reflect.Zero(value.Type()) + } + result := reflect.MakeMapWithSize(value.Type(), value.Len()) + iterator := value.MapRange() + for iterator.Next() { + result.SetMapIndex( + iterator.Key(), + interpolateDatabaseConfigReflect(iterator.Value(), interpolate), + ) + } + return result + case reflect.Slice: + if value.IsNil() { + return reflect.Zero(value.Type()) + } + result := reflect.MakeSlice(value.Type(), value.Len(), value.Len()) + for i := 0; i < value.Len(); i++ { + result.Index(i).Set(interpolateDatabaseConfigReflect(value.Index(i), interpolate)) + } + return result + case reflect.Array: + result := reflect.New(value.Type()).Elem() + for i := 0; i < value.Len(); i++ { + result.Index(i).Set(interpolateDatabaseConfigReflect(value.Index(i), interpolate)) + } + return result + case reflect.Struct: + result := reflect.New(value.Type()).Elem() + result.Set(value) + for i := 0; i < value.NumField(); i++ { + if !value.Type().Field(i).IsExported() { + continue + } + result.Field(i).Set(interpolateDatabaseConfigReflect(value.Field(i), interpolate)) + } + return result + case reflect.Ptr: + if value.IsNil() { + return reflect.Zero(value.Type()) + } + result := reflect.New(value.Type().Elem()) + result.Elem().Set(interpolateDatabaseConfigReflect(value.Elem(), interpolate)) + if result.Type() != value.Type() { + result = result.Convert(value.Type()) + } + return result + default: + return value + } +} diff --git a/dbconn/factory.go b/dbconn/factory.go index 8344654..eabb49a 100644 --- a/dbconn/factory.go +++ b/dbconn/factory.go @@ -275,7 +275,7 @@ func (f factory) Make(cfg blip.ConfigMonitor) (*sql.DB, string, error) { // credentials are fetched via a reload func, even a static credential specified // in the Blip config file. func (f factory) Credentials(cfg blip.ConfigMonitor) (CredentialFunc, error) { - credentialFunc, selected, err := credentials.NewFactory(f.awsConfig, f.passwordSecretParser).Dynamic(cfg) + credentialFunc, selected, err := credentials.NewFactory(f.awsConfig, f.passwordSecretParser).Dynamic(cfg, "3306") if err != nil || selected { return credentialFunc, err } diff --git a/docs/content/config/config-file.md b/docs/content/config/config-file.md index 3848a63..1bb5d7d 100644 --- a/docs/content/config/config-file.md +++ b/docs/content/config/config-file.md @@ -761,7 +761,28 @@ Section [`exporter`](#exporter) is exactly the same in a monitor. Refer to [Monitor Defaults](#monitor-defaults) for configuring MySQL instances, and remember: [`mysql`](#mysql) variables are top-level in a monitor (omit `mysql:` and include the variables directly). -Monitors have three variables that only appear in monitors: `id`, `meta`, and `plan`. +All monitors have three variables that only appear in monitor entries: `id`, +`meta`, and `plan`. A binary that enables an external database module can also +use `database-type` and `database-config`: + +```yaml +monitors: + - id: external + database-type: my-database + hostname: database.example:1234 + database-config: + module-option: value +``` + +An omitted `database-type` retains Blip's built-in MySQL behavior. The type is +a literal module identifier, although direct environment-variable interpolation +such as `${DATABASE_TYPE}` is supported. Monitor-field interpolation is not +supported for this structural value. + +The contents of `database-config` belong to the selected module. Blip +interpolates string values, redacts the complete section from logged config, +and asks the registered module to validate it. Refer to the external module for +its supported fields. Blip itself does not register a non-MySQL module. ### `id` diff --git a/docs/content/config/heartbeat.md b/docs/content/config/heartbeat.md index cb0c69d..aa7755b 100644 --- a/docs/content/config/heartbeat.md +++ b/docs/content/config/heartbeat.md @@ -6,6 +6,11 @@ Although MySQL has built-in replication heartbeats and lag metrics, they are not For example, `Seconds_Behind_Source` from `SHOW REPLICA STATUS` (or `Seconds_Behind_Master` from `SHOW SLAVE STATUS` before MySQL 8.022) is always on but infamously inaccurate: it reports zero when a network issue blocks replication. Consequently, external replication heartbeats are an industry norm because they are easy and accurate—and they work the same across all versions and distributions of MySQL, including the the cloud. +{{< hint type=note >}} +Blip heartbeat is MySQL-specific. External database monitors do not inherit +global heartbeat defaults and reject monitor-level heartbeat configuration. +{{< /hint >}} + ## Quick Start Presuming one source MySQL instance and one read-only replica, the minimal configuration is: diff --git a/docs/content/develop/collectors.md b/docs/content/develop/collectors.md index 807b2b0..7bc2999 100644 --- a/docs/content/develop/collectors.md +++ b/docs/content/develop/collectors.md @@ -133,10 +133,14 @@ preparation: ```go func (myFactory) DatabaseTypes(string) []blip.DatabaseType { - return []blip.DatabaseType{blip.DatabaseTypePostgres} + return []blip.DatabaseType{"my-database"} } ``` +A collector that does not depend on the monitor's database engine returns +`blip.DatabaseTypeAny`. The database-neutral type cannot be combined with +specific database types. + ## Long-running As of Blip v1.2.0, long-running collectors are possible using one of two approaches: diff --git a/docs/content/develop/database-modules.md b/docs/content/develop/database-modules.md new file mode 100644 index 0000000..9024bb4 --- /dev/null +++ b/docs/content/develop/database-modules.md @@ -0,0 +1,77 @@ +--- +--- + +Blip is purpose-built for MySQL. An external database module can reuse its +monitor, plan, collection, transformation, and sink runtime without adding that +database implementation to Blip itself. + +{{< toc >}} + +## Enable Before Boot + +An external module is enabled by the integrating binary before `server.Boot`. +The module must: + +1. Register its database type with `blip.RegisterDatabaseModule`. +2. Register its collectors through the existing `metrics.Register` API. +3. Decorate `Factories.DbConn` with its connection factory. + +The module's connection factory should handle only its own database type and +delegate every other type to the previous factory. It must also preserve +`DbProviderFactory` delegation when the previous factory implements that +optional capability. This convention allows multiple external modules to +compose without changing Blip's built-in MySQL factory. + +## Configuration + +Every external monitor sets a literal `database-type` and can provide an opaque +`database-config` map: + +```yaml +monitors: + - database-type: my-database + hostname: database.example:1234 + username: metrics + database-config: + module-option: value +``` + +An omitted database type remains MySQL. Direct `${ENV_VAR}` interpolation is +supported in `database-type`; monitor-field interpolation is intentionally not +supported because the type selects defaults before the rest of monitor +initialization. + +Blip recursively interpolates string values inside `database-config` and +redacts the entire opaque map when logging monitor configuration. A module uses +`blip.DecodeDatabaseConfig` to strictly decode the map into its own typed +configuration, then applies and validates its defaults in module code. Its +`DatabaseModule.ValidateConfig` implementation provides early validation during +monitor loading. + +MySQL socket, `my.cnf`, heartbeat, plan changing, plan-table storage, and +`mysqld_exporter` emulation are rejected for external database monitors. + +## Connections and Credentials + +`DbProviderFactory` is optional. A module that needs multiple connection pools +returns a `DbProvider`; Blip uses `Primary` for ordinary collectors and closes +the provider only after monitor subsystems and collectors stop. A specialized +collector factory implements `CollectorFactoryWithDBProvider` and type-asserts +the generic provider to a module-owned extension interface. + +The shared `credentials.Factory` supports IAM, Secrets Manager, password files, +static passwords, and passwordless authentication. Its `Dynamic` method takes +the engine's default port explicitly. The module remains responsible for +endpoint normalization, credential caching and refresh, authentication-error +classification, and connection retry behavior. + +## Collector Compatibility + +A module collector implements `CollectorFactoryDatabaseTypes` and returns its +database type. A collector that is independent of the monitor database returns +`DatabaseTypeAny`. A factory that does not implement the optional interface +retains Blip's historical MySQL compatibility. + +Blip validates that database-specific collectors in one plan have a common +database type, and it validates the selected plan against each monitor before +collector preparation. Database-neutral collectors do not constrain the plan. diff --git a/docs/content/plans/changing.md b/docs/content/plans/changing.md index 1394cdc..b5dbaf9 100644 --- a/docs/content/plans/changing.md +++ b/docs/content/plans/changing.md @@ -4,6 +4,11 @@ title: "Changing" Plan changing makes Blip change plans while running (without restarting) based on the state of MySQL: +{{< hint type=note >}} +Plan changing is MySQL-specific. External database monitors do not inherit +global plan-changing defaults and reject monitor-level plan-changing configuration. +{{< /hint >}} + |State|Connected to MySQL|Collecting Metrics|Description| |-----|------------------|------------------|-----------| |`offline`|no|no|Completely offline, no connection to MySQL| diff --git a/docs/content/plans/file.md b/docs/content/plans/file.md index 7e4eb4a..7fe90c2 100644 --- a/docs/content/plans/file.md +++ b/docs/content/plans/file.md @@ -50,9 +50,9 @@ And each domain has a domain-specific configuration that includes: These values are documented for each [domain]({{< ref "/metrics/domains" >}}) and printed on the command line by [`--print-domains`]({{< ref "/config/blip#--print-domains" >}}). All domains in a plan, across every level, must support at least one common -database type. A MySQL-only domain and a domain that supports both MySQL and -PostgreSQL can share a plan, but a MySQL-only domain and a PostgreSQL-only -domain cannot. A shared plan configuration can still contain separate plans +database type. A database-neutral domain can share a plan with any +database-specific domain, but domains for two different database types cannot +share one plan. A shared plan configuration can still contain separate plans for different database types. Since Blip automatically levels up overlapping frequencies (described in [Intro / Plans]({{< ref "intro/plans" >}})), it's conventional to define levels from most to least frequent, as in this example: diff --git a/docs/content/plans/table.md b/docs/content/plans/table.md index 05dfca4..5d6c3dd 100644 --- a/docs/content/plans/table.md +++ b/docs/content/plans/table.md @@ -4,6 +4,13 @@ title: "Table" A plan table contains one plan per row: +{{< hint type=note >}} +Plan-table storage is MySQL-specific. The connection configured by a top-level +`plans.monitor` must be MySQL, and external database monitors reject +`plans.table` in their monitor configuration. External database monitors can +use plan files or compatible shared plans. +{{< /hint >}} + ```sql CREATE TABLE IF NOT EXISTS plans ( name VARCHAR(100) NOT NULL PRIMARY KEY, diff --git a/metrics/factory.go b/metrics/factory.go index 6899fef..b50b4dc 100644 --- a/metrics/factory.go +++ b/metrics/factory.go @@ -80,9 +80,7 @@ func normalizeDatabaseTypes(domain string, databaseTypes []blip.DatabaseType) ([ seen := map[blip.DatabaseType]bool{} normalized := make([]blip.DatabaseType, 0, len(databaseTypes)) for _, databaseType := range databaseTypes { - switch databaseType { - case blip.DatabaseTypeMySQL, blip.DatabaseTypePostgres: - default: + if databaseType != blip.DatabaseTypeAny && !blip.ValidDatabaseType(databaseType) { return nil, fmt.Errorf("collector %s declares invalid database type %q", domain, databaseType) } if seen[databaseType] { @@ -91,6 +89,9 @@ func normalizeDatabaseTypes(domain string, databaseTypes []blip.DatabaseType) ([ seen[databaseType] = true normalized = append(normalized, databaseType) } + if seen[blip.DatabaseTypeAny] && len(normalized) != 1 { + return nil, fmt.Errorf("collector %s declares database-neutral compatibility with specific database types", domain) + } sort.Slice(normalized, func(i, j int) bool { return normalized[i] < normalized[j] }) @@ -155,7 +156,7 @@ func validateDatabase(domain string, databaseType blip.DatabaseType) error { return fmt.Errorf("invalid domain: %s (no factory registered)", domain) } for _, supportedType := range registered.databaseTypes { - if supportedType == databaseType { + if supportedType == blip.DatabaseTypeAny || supportedType == databaseType { return nil } } @@ -167,6 +168,17 @@ func validateDatabase(domain string, databaseType blip.DatabaseType) error { // // See types in the blip package for more details. func Make(domain string, args blip.CollectorFactoryArgs) (blip.Collector, error) { + return makeWithDBProvider(domain, args, nil) +} + +// MakeWithDBProvider makes a collector with an optional monitor-owned database +// provider. Registered factories without the optional provider capability +// continue through their historical Make method. +func MakeWithDBProvider(domain string, args blip.CollectorFactoryArgs, provider blip.DbProvider) (blip.Collector, error) { + return makeWithDBProvider(domain, args, provider) +} + +func makeWithDBProvider(domain string, args blip.CollectorFactoryArgs, provider blip.DbProvider) (blip.Collector, error) { r.Lock() defer r.Unlock() registered, ok := r.factory[domain] @@ -181,6 +193,11 @@ func Make(domain string, args blip.CollectorFactoryArgs) (blip.Collector, error) return nil, err } } + if provider != nil { + if providerFactory, ok := registered.factory.(blip.CollectorFactoryWithDBProvider); ok { + return providerFactory.MakeWithDBProvider(domain, args, provider) + } + } return registered.factory.Make(domain, args) } @@ -345,10 +362,7 @@ func InitFactory(factories blip.Factories) { func (f *factory) DatabaseTypes(domain string) []blip.DatabaseType { if domain == awsrds.DOMAIN { - return []blip.DatabaseType{ - blip.DatabaseTypeMySQL, - blip.DatabaseTypePostgres, - } + return []blip.DatabaseType{blip.DatabaseTypeAny} } return []blip.DatabaseType{blip.DatabaseTypeMySQL} } diff --git a/metrics/factory_test.go b/metrics/factory_test.go index 8b60c64..856c689 100644 --- a/metrics/factory_test.go +++ b/metrics/factory_test.go @@ -3,19 +3,50 @@ package metrics_test import ( + "database/sql" "strings" "testing" - "github.com/cashapp/blip" - "github.com/cashapp/blip/metrics" - "github.com/cashapp/blip/test/mock" + "github.com/cashapp/blip/v2" + "github.com/cashapp/blip/v2/metrics" + "github.com/cashapp/blip/v2/test/mock" ) +const externalType blip.DatabaseType = "test-database" + type databaseTypesFactory struct { mock.MetricFactory databaseTypes func(string) []blip.DatabaseType } +type providerFactory struct { + mock.MetricFactory + providerCalls int + provider blip.DbProvider +} + +func (f *providerFactory) MakeWithDBProvider( + domain string, + args blip.CollectorFactoryArgs, + provider blip.DbProvider, +) (blip.Collector, error) { + f.providerCalls++ + f.provider = provider + return f.MetricFactory.Make(domain, args) +} + +type testProvider struct { + primary *sql.DB +} + +func (p testProvider) Primary() *sql.DB { + return p.primary +} + +func (testProvider) Close() error { + return nil +} + func (f databaseTypesFactory) DatabaseTypes(domain string) []blip.DatabaseType { return f.databaseTypes(domain) } @@ -36,20 +67,20 @@ func TestRegisterDefaultsToMySQL(t *testing.T) { t.Fatalf("Make(default mysql): %v", err) } - err := metrics.ValidateDatabase(domain, blip.DatabaseTypePostgres) - if err == nil || !strings.Contains(err.Error(), `does not support database type "postgres" (supported: [mysql])`) { - t.Fatalf("ValidateDatabase(postgres) error = %v", err) + err := metrics.ValidateDatabase(domain, externalType) + if err == nil || !strings.Contains(err.Error(), `does not support database type "test-database" (supported: [mysql])`) { + t.Fatalf("ValidateDatabase(external) error = %v", err) } } func TestRegisterUsesFactoryDatabaseTypes(t *testing.T) { - const domain = "test.postgres-only" + const domain = "test.external-only" factory := databaseTypesFactory{ databaseTypes: func(gotDomain string) []blip.DatabaseType { if gotDomain != domain { t.Fatalf("DatabaseTypes domain = %q, expected %q", gotDomain, domain) } - return []blip.DatabaseType{blip.DatabaseTypePostgres} + return []blip.DatabaseType{externalType} }, } @@ -58,17 +89,17 @@ func TestRegisterUsesFactoryDatabaseTypes(t *testing.T) { } t.Cleanup(func() { metrics.Remove(domain) }) - if err := metrics.ValidateDatabase(domain, blip.DatabaseTypePostgres); err != nil { - t.Fatalf("ValidateDatabase(postgres): %v", err) + if err := metrics.ValidateDatabase(domain, externalType); err != nil { + t.Fatalf("ValidateDatabase(external): %v", err) } if _, err := metrics.Make(domain, blip.CollectorFactoryArgs{ - Config: blip.ConfigMonitor{DatabaseType: blip.DatabaseTypePostgres}, + Config: blip.ConfigMonitor{DatabaseType: externalType}, }); err != nil { - t.Fatalf("Make(postgres): %v", err) + t.Fatalf("Make(external): %v", err) } err := metrics.ValidateDatabase(domain, blip.DatabaseTypeMySQL) - if err == nil || !strings.Contains(err.Error(), `does not support database type "mysql" (supported: [postgres])`) { + if err == nil || !strings.Contains(err.Error(), `does not support database type "mysql" (supported: [test-database])`) { t.Fatalf("ValidateDatabase(mysql) error = %v", err) } if _, err := metrics.Make(domain, blip.CollectorFactoryArgs{}); err == nil { @@ -87,9 +118,9 @@ func TestRegisterSupportsMultipleDatabaseTypes(t *testing.T) { factory := databaseTypesFactory{ databaseTypes: func(string) []blip.DatabaseType { return []blip.DatabaseType{ - blip.DatabaseTypePostgres, + externalType, blip.DatabaseTypeMySQL, - blip.DatabaseTypePostgres, + externalType, } }, } @@ -101,7 +132,7 @@ func TestRegisterSupportsMultipleDatabaseTypes(t *testing.T) { for _, databaseType := range []blip.DatabaseType{ blip.DatabaseTypeMySQL, - blip.DatabaseTypePostgres, + externalType, } { if err := metrics.ValidateDatabase(domain, databaseType); err != nil { t.Fatalf("ValidateDatabase(%s): %v", databaseType, err) @@ -114,7 +145,7 @@ func TestRegisterSupportsMultipleDatabaseTypes(t *testing.T) { } if len(databaseTypes) != 2 || databaseTypes[0] != blip.DatabaseTypeMySQL || - databaseTypes[1] != blip.DatabaseTypePostgres { + databaseTypes[1] != externalType { t.Fatalf("SupportedDatabaseTypes = %v", databaseTypes) } @@ -134,9 +165,21 @@ func TestRegisterValidatesFactoryDatabaseTypes(t *testing.T) { databaseTypes: nil, errorContains: "supports no database types", }, - "invalid": { - databaseTypes: []blip.DatabaseType{"oracle"}, - errorContains: `declares invalid database type "oracle"`, + "empty value": { + databaseTypes: []blip.DatabaseType{""}, + errorContains: `declares invalid database type ""`, + }, + "whitespace": { + databaseTypes: []blip.DatabaseType{" oracle "}, + errorContains: `declares invalid database type " oracle "`, + }, + "uppercase": { + databaseTypes: []blip.DatabaseType{"Oracle"}, + errorContains: `declares invalid database type "Oracle"`, + }, + "neutral with specific": { + databaseTypes: []blip.DatabaseType{blip.DatabaseTypeAny, "oracle"}, + errorContains: "database-neutral compatibility with specific database types", }, } for name, tt := range tests { @@ -178,17 +221,73 @@ func TestRegisterDuplicateDoesNotInspectFactoryDatabaseTypes(t *testing.T) { } } +func TestMakeWithDBProviderUsesOptionalFactoryCapability(t *testing.T) { + const domain = "test.db-provider" + makeCalls := 0 + factory := &providerFactory{ + MetricFactory: mock.MetricFactory{ + MakeFunc: func(string, blip.CollectorFactoryArgs) (blip.Collector, error) { + makeCalls++ + return mock.MetricsCollector{}, nil + }, + }, + } + if err := metrics.Register(domain, factory); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(domain) }) + + provider := testProvider{primary: &sql.DB{}} + if _, err := metrics.MakeWithDBProvider(domain, blip.CollectorFactoryArgs{}, provider); err != nil { + t.Fatal(err) + } + if factory.providerCalls != 1 || makeCalls != 1 { + t.Fatalf("factory calls: provider=%d Make=%d, expected 1 and 1", + factory.providerCalls, makeCalls) + } + if factory.provider != provider { + t.Fatalf("factory provider = %T %p, expected %T %p", + factory.provider, factory.provider, provider, provider) + } +} + +func TestMakePreservesHistoricalFactoryPath(t *testing.T) { + const domain = "test.db-provider-legacy-make" + makeCalls := 0 + factory := &providerFactory{ + MetricFactory: mock.MetricFactory{ + MakeFunc: func(string, blip.CollectorFactoryArgs) (blip.Collector, error) { + makeCalls++ + return mock.MetricsCollector{}, nil + }, + }, + } + if err := metrics.Register(domain, factory); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(domain) }) + + if _, err := metrics.Make(domain, blip.CollectorFactoryArgs{}); err != nil { + t.Fatal(err) + } + if factory.providerCalls != 0 || makeCalls != 1 { + t.Fatalf("factory calls: provider=%d Make=%d, expected 0 and 1", + factory.providerCalls, makeCalls) + } +} + func TestBuiltInCollectorDatabaseCompatibility(t *testing.T) { if err := metrics.ValidateDatabase("status.global", blip.DatabaseTypeMySQL); err != nil { t.Fatalf("status.global with MySQL: %v", err) } - if err := metrics.ValidateDatabase("status.global", blip.DatabaseTypePostgres); err == nil { - t.Fatal("status.global supports PostgreSQL") + if err := metrics.ValidateDatabase("status.global", externalType); err == nil { + t.Fatal("status.global supports an external database") } for _, databaseType := range []blip.DatabaseType{ blip.DatabaseTypeMySQL, - blip.DatabaseTypePostgres, + externalType, + blip.DatabaseType("future-rds-engine"), } { if err := metrics.ValidateDatabase("aws.rds", databaseType); err != nil { t.Fatalf("aws.rds with %s: %v", databaseType, err) diff --git a/monitor/engine.go b/monitor/engine.go index 432f887..3675e7b 100644 --- a/monitor/engine.go +++ b/monitor/engine.go @@ -46,9 +46,10 @@ type collection struct { // stop/destroy the Engine. Like all Monitor components, an Engine is not restarted // or reused, it's recreated if the Monitor is restarted. type Engine struct { - cfg blip.ConfigMonitor - db *sql.DB - monitorId string + cfg blip.ConfigMonitor + db *sql.DB + dbProvider blip.DbProvider + monitorId string // -- event event.MonitorReceiver *sync.Mutex @@ -57,13 +58,19 @@ type Engine struct { collectAt map[string][]*clutch // keyed on level, sorted ascending by CMR checkAt map[string][]*clutch // keyed on level collectionChan chan collection + collectorWG sync.WaitGroup } func NewEngine(cfg blip.ConfigMonitor, db *sql.DB) *Engine { + return newEngineWithDBProvider(cfg, db, nil) +} + +func newEngineWithDBProvider(cfg blip.ConfigMonitor, db *sql.DB, dbProvider blip.DbProvider) *Engine { return &Engine{ - cfg: cfg, - db: db, - monitorId: cfg.MonitorId, + cfg: cfg, + db: db, + dbProvider: dbProvider, + monitorId: cfg.MonitorId, // -- event: event.MonitorReceiver{MonitorId: cfg.MonitorId}, Mutex: &sync.Mutex{}, @@ -109,14 +116,14 @@ func (e *Engine) Prepare(ctx context.Context, plan blip.Plan, before, after func } }() - // Connect to MySQL. DO NOT loop and retry; try once and return on error + // Connect to the database. DO NOT loop and retry; try once and return on error // to let the caller (a LevelCollector.changePlan goroutine) retry with backoff. - status.Monitor(e.monitorId, status.ENGINE_PREPARE, "%s: connect to MySQL", plan.Name) + status.Monitor(e.monitorId, status.ENGINE_PREPARE, "%s: connect to database", plan.Name) dbctx, cancel := context.WithTimeout(ctx, 5*time.Second) err := e.db.PingContext(dbctx) cancel() if err != nil { - lerr = fmt.Errorf("while connecting to MySQL: %s", err) + lerr = fmt.Errorf("while connecting to database: %s", err) return lerr } @@ -143,13 +150,14 @@ func (e *Engine) Prepare(ctx context.Context, plan blip.Plan, before, after func if _, ok := collectors[domain]; ok { continue // already seen } - c, err := metrics.Make( + c, err := metrics.MakeWithDBProvider( domain, blip.CollectorFactoryArgs{ Config: e.cfg, DB: e.db, MonitorId: e.monitorId, }, + e.dbProvider, ) if err != nil { lerr = fmt.Errorf("while making %s collector: %s", domain, err) @@ -295,7 +303,11 @@ func (e *Engine) Collect(emrCtx context.Context, interval uint, levelName string for _, cl := range domains { select { case <-sem: - go cl.collect(*m, sem) + e.collectorWG.Add(1) + go func(cl *clutch, metrics blip.Metrics) { + defer e.collectorWG.Done() + cl.collect(metrics, sem) + }(cl, *m) running[cl.c.Domain()] = true case <-emrCtx.Done(): blip.Debug("EMR timeout starting collectors") @@ -411,13 +423,9 @@ SWEEP: return metrics, fmt.Errorf("%s: failed: zero metrics collected, %d errors", coId, errCount) } -// Stop the engine and cleanup any metrics associated with it. -// TODO: There is a possible race condition when this is called. Since -// Engine.Collect is called as a go-routine, we could have an invocation -// of the function block waiting for Engine.Stop to unlock -// after which Collect would run after cleanup has been called. -// This could result in a panic, though that should be caught and logged. -// Since the monitor is stopping anyway this isn't a huge issue. +// Stop the engine and cleanup any metrics associated with it. Collect calls +// are serialized by the engine lock; collectorWG also joins background +// collector runs that outlive the Collect call which started them. func (e *Engine) Stop() { blip.Debug("Engine.Stop called") e.Lock() @@ -433,16 +441,26 @@ func (e *Engine) stopCollectors() { /* -- CALLER MUST LOCK Engine -- */ for _, cl := range e.collectors { cl.Lock() - if !cl.running { - cl.Unlock() + if cl.running { + blip.Debug("%s: %s stopping", e.monitorId, cl.c.Domain()) + if cl.cancel != nil { + cl.cancel() + } + } else { blip.Debug("%s: %s not running", e.monitorId, cl.c.Domain()) - continue } - blip.Debug("%s: %s stopping", e.monitorId, cl.c.Domain()) - cl.cancel() + cl.Unlock() + } + + // Cleanup callbacks can release resources used by collector goroutines, so + // wait until every foreground or background run has observed cancellation. + e.collectorWG.Wait() + for _, cl := range e.collectors { + cl.Lock() if cl.cleanup != nil { blip.Debug("%s: %s cleanup", e.monitorId, cl.c.Domain()) cl.cleanup() + cl.cleanup = nil } cl.Unlock() } diff --git a/monitor/level_collector.go b/monitor/level_collector.go index 27f5ec2..941ee39 100644 --- a/monitor/level_collector.go +++ b/monitor/level_collector.go @@ -23,12 +23,12 @@ import ( // // The term "collector" is a little misleading because the LCO doesn't collect // metrics, but it is the first step in the metrics collection process, which -// looks roughly like: LCO -> Engine -> metric collectors -> MySQL. +// looks roughly like: LCO -> Engine -> metric collectors -> database. // In Run, the LCO checks every 1s for the highest level in the plan to collect. // For example, after 5s it'll collect levels with a frequency divisible by 5s. // See https://block.github.io/blip/plans/file/. // -// Metrics from MySQL flow back to the LCO as blip.Metrics, which the LCO +// Metrics from the database flow back to the LCO as blip.Metrics, which the LCO // passes to blip.Plugin.TransformMetrics if specified, then to all sinks // specified for the monitor. type LevelCollector interface { @@ -78,6 +78,10 @@ type LevelCollectorArgs struct { } func NewLevelCollector(args LevelCollectorArgs) *lco { + return newLevelCollectorWithDBProvider(args, nil) +} + +func newLevelCollectorWithDBProvider(args LevelCollectorArgs, dbProvider blip.DbProvider) *lco { return &lco{ cfg: args.Config, planLoader: args.PlanLoader, @@ -85,7 +89,7 @@ func NewLevelCollector(args LevelCollectorArgs) *lco { transformMetrics: args.TransformMetrics, // -- monitorId: args.Config.MonitorId, - engine: NewEngine(args.Config, args.DB), + engine: newEngineWithDBProvider(args.Config, args.DB, dbProvider), stateMux: &sync.Mutex{}, paused: true, changeMux: &sync.Mutex{}, @@ -215,15 +219,7 @@ func (c *lco) Run(stopChan, doneChan chan struct{}) error { break } blip.Debug("stopChan closed at %s s=%d interval=%d", startTime, s, interval) - c.changeMux.Lock() - defer c.changeMux.Unlock() - c.stopped = true // make ChangePlan do nothing - select { - case <-c.changePlanDoneChan: - c.changePlanCancelFunc() // stop --> changePlan goroutine - <-c.changePlanDoneChan // wait for changePlan goroutine - default: - } + c.stopChangePlan() c.engine.Stop() // stop all collectors and run their cleanup func return nil default: // no @@ -259,6 +255,23 @@ func (c *lco) Run(stopChan, doneChan chan struct{}) error { return nil } +// stopChangePlan prevents new plan changes, cancels the current plan change, +// and waits for its worker to stop using the engine and database provider. +func (c *lco) stopChangePlan() { + c.changeMux.Lock() + defer c.changeMux.Unlock() + + c.stopped = true + if c.changePlanCancelFunc == nil { + return + } + + c.changePlanCancelFunc() + <-c.changePlanDoneChan + c.changePlanCancelFunc = nil + c.changePlanDoneChan = nil +} + func (c *lco) collect(interval uint, levelName string, startTime time.Time) { status.Monitor(c.monitorId, status.LEVEL_COLLECT, "%s/%s: collecting", c.plan.Name, levelName) defer func() { @@ -357,7 +370,7 @@ func (c *lco) ChangePlan(newState, newPlanName string) error { // changePlan is a gorountine run by ChangePlan It's potentially long-running // because it waits for Engine.Prepare. If that function returns an error -// (e.g. MySQL is offline), then this function retires forever, or until canceled +// (e.g. the database is offline), then this function retries forever, or until canceled // by either another call to ChangePlan or Run is stopped (LCO is terminated). // // Never all this function directly; it's only called via ChangePlan, which @@ -389,7 +402,10 @@ func (c *lco) changePlan(ctx context.Context, doneChan chan struct{}, newState, errMsg := fmt.Sprintf("%s: error loading new plan %s: %s (retrying)", change, newPlanName, err) status.Monitor(c.monitorId, status.LEVEL_CHANGE_PLAN, "%s", errMsg) c.event.Sendf(event.CHANGE_PLAN_ERROR, "%s", errMsg) - time.Sleep(2 * time.Second) + if !waitForChangePlanRetry(ctx, 2*time.Second) { + blip.Debug("changePlan canceled while loading plan") + return + } } change = fmt.Sprintf("state:%s plan:%s -> state:%s plan:%s", oldState, oldPlanName, newState, newPlan.Name) @@ -438,9 +454,9 @@ func (c *lco) changePlan(ctx context.Context, doneChan chan struct{}, newState, c.stateMux.Unlock() // -- X unlock -- } - // Try forever, or until context is cancelled, because it could be that MySQL is + // Try forever, or until context is cancelled, because it could be that the database is // temporarily offline. In the real world, this is not uncommon: Blip might be - // started before MySQL, for example. We're running in a goroutine from ChangePlan + // started before the database, for example. We're running in a goroutine from ChangePlan // that already returned to its caller, so we're not blocking anything here. // More importantly, as documented in several place: this is _the code_ that // all other code relies on to try "forever" because a plan must be prepared @@ -464,13 +480,28 @@ func (c *lco) changePlan(ctx context.Context, doneChan chan struct{}, newState, return // changePlan goroutine has been cancelled } status.Monitor(c.monitorId, status.LEVEL_CHANGE_PLAN, "%s: error preparing new plan %s: %s (retrying)", change, newPlan.Name, err) - time.Sleep(retry.NextBackOff()) + if !waitForChangePlanRetry(ctx, retry.NextBackOff()) { + blip.Debug("changePlan canceled while waiting to retry plan preparation") + return + } } status.RemoveComponent(c.monitorId, status.LEVEL_CHANGE_PLAN) c.event.Sendf(event.CHANGE_PLAN_SUCCESS, "%s", change) } +func waitForChangePlanRetry(ctx context.Context, delay time.Duration) bool { + timer := time.NewTimer(delay) + defer timer.Stop() + + select { + case <-ctx.Done(): + return false + case <-timer.C: + return true + } +} + // Pause pauses metrics collection until ChangePlan is called. Run still runs, // but it doesn't collect when paused. The only way to resume after pausing is // to call ChangePlan again. diff --git a/monitor/monitor.go b/monitor/monitor.go index 358140d..022fc45 100644 --- a/monitor/monitor.go +++ b/monitor/monitor.go @@ -1,7 +1,7 @@ // Copyright 2024 Block, Inc. // Package monitor provides core Blip components that, together, monitor one -// MySQL instance. Most monitoring logic happens in the package, but package +// database target. Most monitoring logic happens in the package, but package // metrics is closely related: this latter actually collect metrics, but it // is driven by this package. Other Blip packages are mostly set up and support // of monitors. @@ -25,7 +25,7 @@ import ( "github.com/cashapp/blip/v2/status" ) -// Monitor monitors one MySQL instance. The monitor is a high-level component +// Monitor monitors one database target. The monitor is a high-level component // that runs (and keeps running) four monitor subsystems: // - Plan changer (PCH) // - Level collector (LCO) @@ -37,7 +37,7 @@ import ( // If any subsystem crashes (returns for any reason or panics), the monitor // stops and restarts all subsystems. The monitor doesn't stop until Stop is // called. Consequently, if a monitor is not configured correctly (for example, -// it can't connect to MySQL), it tries and reports every forever. +// it can't connect to the database), it tries and reports every forever. // // Monitors are loaded, created, and initially started only by the MonitorLoader. // A monitor can be stopped and started (again) via the server API. @@ -58,13 +58,15 @@ type Monitor struct { transformMetric func([]*blip.Metrics) error // Core components - runMux *sync.RWMutex - db *sql.DB - dsn string // redacted (no password) - promAPI *prom.API - lco LevelCollector - pch PlanChanger - hbw *heartbeat.Writer + runMux *sync.RWMutex + db *sql.DB + dbProvider blip.DbProvider + dsn string // redacted (no password) + promAPI *prom.API + exporter *Exporter + lco LevelCollector + pch PlanChanger + hbw *heartbeat.Writer // Control chans and sync runLoopChan chan struct{} // Stop(): stop the monitor @@ -88,7 +90,7 @@ type MonitorArgs struct { // NewMonitor creates a new Monitor with the given arguments. The caller must // call Boot then, if that does not return an error, Run to start monitoring -// the MySQL instance. +// the database target. func NewMonitor(args MonitorArgs) *Monitor { retry := backoff.NewExponentialBackOff() retry.MaxElapsedTime = 0 @@ -150,10 +152,8 @@ func (m *Monitor) Stop() error { // Stop and wait for monitor subsystems m.stop(false, "Stop") - // Everything should be stopped now, so close db connection - if m.db != nil { - m.db.Close() - } + // Everything should be stopped now, so close database resources. + m.closeDB() event.Sendf(event.MONITOR_STOPPED, "%s", m.monitorId) status.Monitor(m.monitorId, status.MONITOR, "stopped at %s", blip.FormatTime(time.Now())) @@ -232,7 +232,7 @@ func (m *Monitor) runLoop() { // then runLoop() calls startup again to restart monitoring. // // startup is called only by runLoop, which guards (serializes) and monitors it. -func (m *Monitor) startup() error { +func (m *Monitor) startup() (err error) { blip.Debug("%s: startup call", m.monitorId) defer blip.Debug("%s: startup return", m.monitorId) @@ -249,15 +249,22 @@ func (m *Monitor) startup() error { // DB-plan loop // ////////////////////////////////////////////////////////////////////// + // Release database resources from an earlier failed startup or subsystem + // restart before creating the next monitor-owned connection set. + m.runMux.Lock() + m.closeDB() + m.runMux.Unlock() + // ---------------------------------------------------------------------- - // Make DSN and *sql.DB. This does NOT connect to MySQL. + // Make DSN and *sql.DB. This does NOT connect to the database. for { status.Monitor(m.monitorId, status.MONITOR, "making DB/DSN (not connecting)") - db, dsnRedacted, err := m.dbMaker.Make(m.cfg) + dbProvider, db, dsnRedacted, err := m.makeDB() m.setErr(err, false) if err == nil { // success m.runMux.Lock() m.db = db + m.dbProvider = dbProvider m.dsn = dsnRedacted status.Monitor(m.monitorId, status.MONITOR_DSN, "%s", dsnRedacted) m.runMux.Unlock() @@ -273,7 +280,7 @@ func (m *Monitor) startup() error { } // ---------------------------------------------------------------------- - // Load monitor plans, if any. This MIGHT connect to MySQL if the plan + // Load monitor plans, if any. This MIGHT connect to the database if the plan // is stored in a table. for { status.Monitor(m.monitorId, status.MONITOR, "loading plans") @@ -297,6 +304,11 @@ func (m *Monitor) startup() error { m.runMux.Lock() defer m.runMux.Unlock() + defer func() { + if err != nil { + m.stop(false, "startup error") + } + }() // ---------------------------------------------------------------------- // Heartbeat @@ -307,17 +319,19 @@ func (m *Monitor) startup() error { if m.cfg.Heartbeat.Freq != "" { status.Monitor(m.monitorId, status.MONITOR, "starting heartbeat") m.hbw = heartbeat.NewWriter(m.monitorId, m.db, m.cfg.Heartbeat) + hbw := m.hbw + runChan := m.runChan m.wg.Add(1) go func() { - defer m.stop(true, "heartbeat.Writer") // stop monitor subsystems - defer m.wg.Done() // notify stop() - defer func() { // catch panic in heartbeat.Writer + defer m.stopRun(runChan, "heartbeat.Writer") // stop monitor subsystems + defer m.wg.Done() // notify stop() + defer func() { // catch panic in heartbeat.Writer if r := recover(); r != nil { m.panic(r) } }() doneChan := make(chan struct{}) // Monitor uses wg - m.hbw.Write(m.runChan, doneChan) + hbw.Write(runChan, doneChan) }() } @@ -349,23 +363,26 @@ func (m *Monitor) startup() error { } // Run API to emulate an exporter, responding to GET /metrics + m.exporter = NewExporter(m.cfg.Exporter, promPlan, newEngineWithDBProvider(m.cfg, m.db, m.dbProvider)) m.promAPI = prom.NewAPI( m.cfg.Exporter, m.monitorId, - NewExporter(m.cfg.Exporter, promPlan, NewEngine(m.cfg, m.db)), + m.exporter, ) + promAPI := m.promAPI + runChan := m.runChan m.wg.Add(1) go func() { defer status.RemoveComponent(m.monitorId, "exporter") - defer m.stop(true, "prom.API") // stop monitor subsystems - defer m.wg.Done() // notify stop() - defer func() { // catch panic in exporter API + defer m.stopRun(runChan, "prom.API") // stop monitor subsystems + defer m.wg.Done() // notify stop() + defer func() { // catch panic in exporter API if r := recover(); r != nil { m.panic(r) } }() - err := m.promAPI.Run() + err := promAPI.Run() if err == nil { // shutdown blip.Debug("%s: prom api stopped", m.monitorId) return @@ -391,25 +408,27 @@ func (m *Monitor) startup() error { // config.plans.change, then it will do this; if it's not enabled, // we'll do it as the last startup step. status.Monitor(m.monitorId, status.MONITOR, "starting level collector") - m.lco = NewLevelCollector(LevelCollectorArgs{ + m.lco = newLevelCollectorWithDBProvider(LevelCollectorArgs{ Config: m.cfg, DB: m.db, PlanLoader: m.planLoader, Sinks: m.sinks, TransformMetrics: m.transformMetric, - }) + }, m.dbProvider) + lco := m.lco + runChan := m.runChan m.wg.Add(1) go func() { - defer m.stop(true, "LCO") // stop monitor subsystems - defer m.wg.Done() // notify stop() - defer func() { // catch panic in LCO + defer m.stopRun(runChan, "LCO") // stop monitor subsystems + defer m.wg.Done() // notify stop() + defer func() { // catch panic in LCO if r := recover(); r != nil { m.panic(r) } }() doneChan := make(chan struct{}) // Monitor uses wg - m.lco.Run(m.runChan, doneChan) + lco.Run(runChan, doneChan) }() // ---------------------------------------------------------------------- @@ -427,18 +446,19 @@ func (m *Monitor) startup() error { LCO: m.lco, HA: m.ha, }) + pch := m.pch m.wg.Add(1) go func() { - defer m.stop(true, "PCH") // stop monitor subsystems - defer m.wg.Done() // notify stop() - defer func() { // catch panic in PCH + defer m.stopRun(runChan, "PCH") // stop monitor subsystems + defer m.wg.Done() // notify stop() + defer func() { // catch panic in PCH if r := recover(); r != nil { m.panic(r) } }() doneChan := make(chan struct{}) // Monitor uses wg - m.pch.Run(m.runChan, doneChan) // start LCO indirectly + pch.Run(runChan, doneChan) // start LCO indirectly }() } else { // When the PCH is not enabled, we must init the state and plan, @@ -456,6 +476,46 @@ func (m *Monitor) startup() error { return nil } +func (m *Monitor) makeDB() (blip.DbProvider, *sql.DB, string, error) { + providerFactory, ok := m.dbMaker.(blip.DbProviderFactory) + if !ok { + db, dsn, err := m.dbMaker.Make(m.cfg) + return nil, db, dsn, err + } + + provider, dsn, err := providerFactory.MakeProvider(m.cfg) + if err != nil { + if provider != nil { + provider.Close() + } + return nil, nil, "", err + } + if provider == nil { + return nil, nil, "", fmt.Errorf("database provider factory returned a nil provider") + } + db := provider.Primary() + if db == nil { + provider.Close() + return nil, nil, "", fmt.Errorf("database provider returned a nil primary connection") + } + return provider, db, dsn, nil +} + +// closeDB releases the current monitor-owned database resources. The caller +// must hold runMux. +func (m *Monitor) closeDB() { + if m.dbProvider != nil { + m.dbProvider.Close() + m.dbProvider = nil + m.db = nil + return + } + if m.db != nil { + m.db.Close() + m.db = nil + } +} + // stop stops the monitor subsystems started in startup. It does not stop the // monitor; Stop does that. Stopping only the monitor subsystems causes runLoop // to restart them. @@ -482,11 +542,34 @@ func (m *Monitor) stop(lock bool, caller string) { // it's running an http.Server if m.promAPI != nil { m.promAPI.Stop() + m.promAPI = nil } // Wait for monitor subsystem goroutines to return status.Monitor(m.monitorId, status.MONITOR, "stopping goroutines") m.wg.Wait() + if m.exporter != nil { + m.exporter.Stop() + m.exporter = nil + } + + // A subsystem failure restarts the whole monitor, including its database + // provider. Close the current provider only after every subsystem has + // stopped using its connections. + m.closeDB() +} + +// stopRun stops a subsystem generation only if it is still current. A +// subsystem from an earlier startup can finish after runLoop has installed a +// new run channel; it must not stop that newer generation. +func (m *Monitor) stopRun(runChan chan struct{}, caller string) { + m.runMux.Lock() + defer m.runMux.Unlock() + if m.runChan != runChan { + blip.Debug("%s: stop called by %s for obsolete run (noop)", m.monitorId, caller) + return + } + m.stop(false, caller) } func (m *Monitor) setErr(err error, isPanic bool) { diff --git a/monitor/mysqld_exporter.go b/monitor/mysqld_exporter.go index 419a3b7..038fd6e 100644 --- a/monitor/mysqld_exporter.go +++ b/monitor/mysqld_exporter.go @@ -107,3 +107,13 @@ func (e Exporter) Collect(ch chan<- prometheus.Metric) { tr.Translate(vals, ch) } } + +// Stop releases collector resources prepared by the exporter engine. The +// monitor calls this only after the HTTP API has stopped accepting and serving +// scrapes. +func (e *Exporter) Stop() { + e.Lock() + defer e.Unlock() + e.engine.Stop() + e.prepared = false +} diff --git a/monitor/provider_test.go b/monitor/provider_test.go new file mode 100644 index 0000000..6bee7ad --- /dev/null +++ b/monitor/provider_test.go @@ -0,0 +1,585 @@ +// Copyright 2026 Block, Inc. + +package monitor + +import ( + "context" + "database/sql" + "errors" + "fmt" + "strings" + "sync" + "testing" + "time" + + "github.com/cashapp/blip/v2" + "github.com/cashapp/blip/v2/heartbeat" + "github.com/cashapp/blip/v2/metrics" + "github.com/cashapp/blip/v2/plan" + "github.com/cashapp/blip/v2/test" + "github.com/cashapp/blip/v2/test/mock" +) + +func TestMonitorMakeDBUsesProviderFactory(t *testing.T) { + primary := &sql.DB{} + provider := &testDBProvider{primary: primary} + factory := &testDBProviderFactory{ + provider: provider, + dsn: "redacted", + } + monitor := NewMonitor(MonitorArgs{ + Config: blip.ConfigMonitor{MonitorId: "provider"}, + DbMaker: factory, + }) + + gotProvider, gotPrimary, gotDSN, err := monitor.makeDB() + if err != nil { + t.Fatal(err) + } + if gotProvider != provider { + t.Fatalf("provider = %T %p, expected %T %p", gotProvider, gotProvider, provider, provider) + } + if gotPrimary != primary { + t.Fatalf("primary = %p, expected %p", gotPrimary, primary) + } + if gotDSN != "redacted" { + t.Fatalf("DSN = %q, expected redacted", gotDSN) + } + if factory.makeCalls != 0 || factory.makeProviderCalls != 1 { + t.Fatalf("factory calls: Make=%d MakeProvider=%d, expected 0 and 1", + factory.makeCalls, factory.makeProviderCalls) + } +} + +func TestMonitorMakeDBPreservesLegacyFactory(t *testing.T) { + primary := &sql.DB{} + factory := &testLegacyDBFactory{primary: primary, dsn: "legacy"} + monitor := NewMonitor(MonitorArgs{ + Config: blip.ConfigMonitor{MonitorId: "legacy"}, + DbMaker: factory, + }) + + provider, gotPrimary, gotDSN, err := monitor.makeDB() + if err != nil { + t.Fatal(err) + } + if provider != nil { + t.Fatalf("legacy factory returned provider %T, expected nil", provider) + } + if gotPrimary != primary || gotDSN != "legacy" { + t.Fatalf("legacy result = (%p, %q), expected (%p, legacy)", gotPrimary, gotDSN, primary) + } +} + +func TestMonitorMakeDBRejectsInvalidProvider(t *testing.T) { + tests := []struct { + name string + provider blip.DbProvider + factoryErr error + wantError string + }{ + { + name: "factory error", + provider: &testDBProvider{primary: &sql.DB{}}, + factoryErr: errors.New("make failed"), + wantError: "make failed", + }, + { + name: "nil provider", + wantError: "nil provider", + }, + { + name: "nil primary", + provider: &testDBProvider{}, + wantError: "nil primary", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + factory := &testDBProviderFactory{ + provider: test.provider, + err: test.factoryErr, + } + monitor := NewMonitor(MonitorArgs{ + Config: blip.ConfigMonitor{MonitorId: test.name}, + DbMaker: factory, + }) + + _, _, _, err := monitor.makeDB() + if err == nil || !errors.Is(err, test.factoryErr) && !strings.Contains(err.Error(), test.wantError) { + t.Fatalf("makeDB error = %v, expected %q", err, test.wantError) + } + if provider, ok := test.provider.(*testDBProvider); ok && provider.closeCalls != 1 { + t.Fatalf("provider Close calls = %d, expected 1", provider.closeCalls) + } + }) + } +} + +func TestMonitorCloseDBClosesProviderOnce(t *testing.T) { + provider := &testDBProvider{primary: &sql.DB{}} + monitor := &Monitor{ + db: provider.primary, + dbProvider: provider, + } + + monitor.closeDB() + monitor.closeDB() + + if provider.closeCalls != 1 { + t.Fatalf("provider Close calls = %d, expected 1", provider.closeCalls) + } + if monitor.db != nil || monitor.dbProvider != nil { + t.Fatalf("monitor retained database resources: db=%p provider=%T", monitor.db, monitor.dbProvider) + } +} + +func TestMonitorStopWaitsForPlanPreparationBeforeClosingProvider(t *testing.T) { + TickerDuration(10*time.Millisecond, time.Second) + defer TickerDuration(time.Second, time.Second) + + const ( + domain = "test.provider-stop" + monitorID = "provider-stop" + planName = "provider-stop-plan" + levelName = "provider-stop-level" + stopCaller = "provider-stop-test" + ) + + prepareStarted := make(chan struct{}) + prepareStopped := make(chan struct{}) + releasePrepare := make(chan struct{}) + collector := mock.MetricsCollector{ + DomainFunc: func() string { return domain }, + PrepareFunc: func(ctx context.Context, _ blip.Plan) (func(), error) { + close(prepareStarted) + select { + case <-ctx.Done(): + close(prepareStopped) + return nil, ctx.Err() + case <-releasePrepare: + close(prepareStopped) + return nil, nil + } + }, + } + if err := metrics.Register(domain, mock.MetricFactory{ + MakeFunc: func(string, blip.CollectorFactoryArgs) (blip.Collector, error) { + return collector, nil + }, + }); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(domain) }) + + loader := plan.NewLoader(func(blip.ConfigPlans) ([]blip.Plan, error) { + return []blip.Plan{{ + Name: planName, + Levels: map[string]blip.Level{ + levelName: { + Name: levelName, + Freq: "1s", + Collect: map[string]blip.Domain{ + domain: {}, + }, + }, + }, + }}, nil + }) + if err := loader.LoadShared(blip.ConfigPlans{}, nil); err != nil { + t.Fatal(err) + } + config := blip.ConfigMonitor{MonitorId: monitorID} + if err := loader.LoadMonitor(config, nil); err != nil { + t.Fatal(err) + } + + _, primary, err := test.Connection(test.DefaultMySQLVersion) + if err != nil { + if test.Build { + t.Skip("MySQL test fixture not running") + } + t.Fatal(err) + } + defer primary.Close() + provider := &testDBProvider{ + primary: primary, + closeFunc: func() { + select { + case <-prepareStopped: + default: + close(releasePrepare) + t.Error("database provider closed before plan preparation stopped") + } + }, + } + lco := newLevelCollectorWithDBProvider(LevelCollectorArgs{ + Config: config, + DB: primary, + PlanLoader: loader, + }, provider) + monitor := NewMonitor(MonitorArgs{Config: config}) + monitor.runChan = make(chan struct{}) + monitor.db = primary + monitor.dbProvider = provider + + lcoDone := make(chan struct{}) + monitor.wg.Add(1) + go func() { + defer monitor.wg.Done() + _ = lco.Run(monitor.runChan, lcoDone) + }() + if err := lco.ChangePlan(blip.STATE_ACTIVE, planName); err != nil { + t.Fatal(err) + } + select { + case <-prepareStarted: + case <-time.After(time.Second): + t.Fatal("timeout waiting for plan preparation to start") + } + + stopDone := make(chan struct{}) + go func() { + monitor.stop(false, stopCaller) + close(stopDone) + }() + select { + case <-stopDone: + case <-time.After(2 * time.Second): + select { + case <-prepareStopped: + default: + close(releasePrepare) + } + t.Fatal("timeout waiting for monitor to stop") + } + + select { + case <-prepareStopped: + default: + t.Fatal("plan preparation was still running after monitor stop") + } + if provider.closeCalls != 1 { + t.Fatalf("provider Close calls = %d, expected 1", provider.closeCalls) + } +} + +func TestMonitorStartupErrorStopsPartialSubsystemsBeforeClosingProvider(t *testing.T) { + const ( + monitorID = "partial-startup" + exporterPlan = "invalid-exporter" + heartbeatDB = "blip_monitor_provider_test" + heartbeatTable = heartbeatDB + ".heartbeat" + ) + + _, primary, err := test.Connection(test.DefaultMySQLVersion) + if err != nil { + if test.Build { + t.Skip("MySQL test fixture not running") + } + t.Fatal(err) + } + defer primary.Close() + if _, err := primary.Exec("CREATE DATABASE IF NOT EXISTS " + heartbeatDB); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _, _ = primary.Exec("DROP DATABASE IF EXISTS " + heartbeatDB) }) + heartbeatDDL := strings.Replace(heartbeat.BLIP_TABLE_DDL, "heartbeat", heartbeatTable, 1) + if _, err := primary.Exec(heartbeatDDL); err != nil { + t.Fatal(err) + } + + loader := plan.NewLoader(func(blip.ConfigPlans) ([]blip.Plan, error) { + return []blip.Plan{{ + Name: exporterPlan, + Levels: map[string]blip.Level{ + "fast": {Name: "fast", Freq: "1s"}, + "slow": {Name: "slow", Freq: "5s"}, + }, + }}, nil + }) + if err := loader.LoadShared(blip.ConfigPlans{}, nil); err != nil { + t.Fatal(err) + } + + config := blip.ConfigMonitor{ + MonitorId: monitorID, + Heartbeat: blip.ConfigHeartbeat{ + Freq: "10ms", + Table: heartbeatTable, + }, + Exporter: blip.ConfigExporter{ + Mode: blip.EXPORTER_MODE_DUAL, + Plan: exporterPlan, + }, + } + provider := &testDBProvider{primary: primary} + monitor := NewMonitor(MonitorArgs{ + Config: config, + DbMaker: &testDBProviderFactory{provider: provider, dsn: "redacted"}, + PlanLoader: loader, + }) + monitor.runLoopChan = make(chan struct{}) + monitor.runChan = make(chan struct{}) + provider.closeFunc = func() { + select { + case <-monitor.runChan: + default: + t.Error("database provider closed before partial subsystems were stopped") + } + } + + err = monitor.startup() + if err == nil || !strings.Contains(err.Error(), "expected 1") { + t.Fatalf("startup error = %v, expected invalid exporter level count", err) + } + if provider.closeCalls != 1 { + t.Fatalf("provider Close calls = %d, expected 1", provider.closeCalls) + } + if monitor.db != nil || monitor.dbProvider != nil { + t.Fatalf("monitor retained database resources: db=%p provider=%T", monitor.db, monitor.dbProvider) + } +} + +func TestMonitorStopRunIgnoresObsoleteGeneration(t *testing.T) { + oldRun := make(chan struct{}) + currentRun := make(chan struct{}) + monitor := NewMonitor(MonitorArgs{Config: blip.ConfigMonitor{MonitorId: "run-generation"}}) + monitor.runChan = currentRun + + monitor.stopRun(oldRun, "obsolete") + + select { + case <-currentRun: + t.Fatal("obsolete subsystem stopped the current run") + default: + } +} + +func TestMonitorStopCleansExporterBeforeClosingProvider(t *testing.T) { + cleaned := false + provider := &testDBProvider{ + primary: &sql.DB{}, + closeFunc: func() { + if !cleaned { + t.Error("database provider closed before exporter cleanup") + } + }, + } + collector := mock.MetricsCollector{DomainFunc: func() string { return "test.exporter-cleanup" }} + engine := newEngineWithDBProvider(blip.ConfigMonitor{MonitorId: "exporter-cleanup"}, provider.primary, provider) + engine.collectors[collector.Domain()] = &clutch{ + c: collector, + cleanup: func() { cleaned = true }, + domain: collector.Domain(), + Mutex: &sync.Mutex{}, + } + monitor := NewMonitor(MonitorArgs{Config: blip.ConfigMonitor{MonitorId: "exporter-cleanup"}}) + monitor.runChan = make(chan struct{}) + monitor.db = provider.primary + monitor.dbProvider = provider + monitor.exporter = NewExporter(blip.ConfigExporter{}, blip.Plan{}, engine) + + monitor.stop(false, "exporter-cleanup-test") + + if !cleaned { + t.Fatal("exporter collector cleanup was not called") + } + if provider.closeCalls != 1 { + t.Fatalf("provider Close calls = %d, expected 1", provider.closeCalls) + } +} + +func TestEngineStopWaitsForBackgroundCollector(t *testing.T) { + const ( + domain = "test.background-stop" + level = "fast" + ) + backgroundStarted := make(chan struct{}) + backgroundStopped := make(chan struct{}) + var collectCtx context.Context + collector := mock.MetricsCollector{ + DomainFunc: func() string { return domain }, + CollectFunc: func(ctx context.Context, _ string) ([]blip.MetricValue, error) { + if ctx != nil { + collectCtx = ctx + return nil, blip.ErrMore + } + close(backgroundStarted) + <-collectCtx.Done() + close(backgroundStopped) + return nil, nil + }, + } + engine := newEngineWithDBProvider(blip.ConfigMonitor{MonitorId: "background-stop"}, &sql.DB{}, nil) + cl := &clutch{ + c: collector, + cleanup: func() { + select { + case <-backgroundStopped: + default: + t.Error("collector cleanup ran before background collection stopped") + } + }, + domain: domain, + cmr: time.Second, + collectionChan: engine.collectionChan, + Mutex: &sync.Mutex{}, + } + engine.plan = blip.Plan{Name: "background", Levels: map[string]blip.Level{level: {Name: level, Freq: "1s"}}} + engine.collectors[domain] = cl + engine.collectAt[level] = []*clutch{cl} + engine.checkAt[level] = nil + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + if _, err := engine.Collect(ctx, 1, level, time.Now()); err != nil { + t.Fatal(err) + } + select { + case <-backgroundStarted: + case <-time.After(time.Second): + t.Fatal("timeout waiting for background collection to start") + } + + engine.Stop() + + select { + case <-backgroundStopped: + default: + t.Fatal("background collector was still running after Engine.Stop") + } +} + +func TestNewEngineWithDBProviderRetainsProvider(t *testing.T) { + provider := &testDBProvider{primary: &sql.DB{}} + engine := newEngineWithDBProvider(blip.ConfigMonitor{MonitorId: "engine"}, provider.primary, provider) + + if engine.DB() != provider.primary { + t.Fatalf("engine primary = %p, expected %p", engine.DB(), provider.primary) + } + if engine.dbProvider != provider { + t.Fatalf("engine provider = %T %p, expected %T %p", engine.dbProvider, engine.dbProvider, provider, provider) + } +} + +func TestEnginePassesProviderToCollectorFactory(t *testing.T) { + _, db, err := test.Connection(test.DefaultMySQLVersion) + if err != nil { + if test.Build { + t.Skip("MySQL test fixture not running") + } + t.Fatal(err) + } + defer db.Close() + + const domain = "test.db-provider" + provider := &testDBProvider{primary: db} + factory := &testProviderCollectorFactory{domain: domain} + if err := metrics.Register(domain, factory); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(domain) }) + + engine := newEngineWithDBProvider( + blip.ConfigMonitor{MonitorId: "provider-engine"}, + db, + provider, + ) + plan := blip.Plan{ + Name: "provider", + Levels: map[string]blip.Level{ + "fast": { + Name: "fast", + Freq: "1s", + Collect: map[string]blip.Domain{ + domain: {}, + }, + }, + }, + } + if err := engine.Prepare(context.Background(), plan, func() {}, func() {}); err != nil { + t.Fatal(err) + } + defer engine.Stop() + + if factory.provider != provider { + t.Fatalf("collector provider = %T %p, expected %T %p", + factory.provider, factory.provider, provider, provider) + } + if factory.makeCalls != 0 || factory.makeProviderCalls != 1 { + t.Fatalf("collector factory calls: Make=%d MakeWithDBProvider=%d, expected 0 and 1", + factory.makeCalls, factory.makeProviderCalls) + } +} + +type testDBProvider struct { + primary *sql.DB + closeCalls int + closeFunc func() +} + +func (p *testDBProvider) Primary() *sql.DB { + return p.primary +} + +func (p *testDBProvider) Close() error { + if p.closeFunc != nil { + p.closeFunc() + } + p.closeCalls++ + return nil +} + +type testDBProviderFactory struct { + provider blip.DbProvider + dsn string + err error + makeCalls int + makeProviderCalls int +} + +func (f *testDBProviderFactory) Make(blip.ConfigMonitor) (*sql.DB, string, error) { + f.makeCalls++ + return nil, "", errors.New("legacy Make should not be called") +} + +func (f *testDBProviderFactory) MakeProvider(blip.ConfigMonitor) (blip.DbProvider, string, error) { + f.makeProviderCalls++ + return f.provider, f.dsn, f.err +} + +type testLegacyDBFactory struct { + primary *sql.DB + dsn string +} + +func (f *testLegacyDBFactory) Make(blip.ConfigMonitor) (*sql.DB, string, error) { + return f.primary, f.dsn, nil +} + +type testProviderCollectorFactory struct { + domain string + provider blip.DbProvider + makeCalls int + makeProviderCalls int +} + +func (f *testProviderCollectorFactory) Make(string, blip.CollectorFactoryArgs) (blip.Collector, error) { + f.makeCalls++ + return nil, errors.New("legacy Make should not be called") +} + +func (f *testProviderCollectorFactory) MakeWithDBProvider( + domain string, + _ blip.CollectorFactoryArgs, + provider blip.DbProvider, +) (blip.Collector, error) { + f.makeProviderCalls++ + f.provider = provider + if domain != f.domain { + return nil, fmt.Errorf("collector domain = %q, expected %q", domain, f.domain) + } + return mock.MetricsCollector{DomainFunc: func() string { return domain }}, nil +} diff --git a/plan/loader.go b/plan/loader.go index 00eefb6..a350db1 100644 --- a/plan/loader.go +++ b/plan/loader.go @@ -609,21 +609,26 @@ func validatePlanDatabaseCompatibility(plan blip.Plan) error { commonTypes := map[blip.DatabaseType]bool{} domainTypes := make([]string, 0, len(domains)) - for i, domain := range domains { + hasDatabaseConstraint := false + for _, domain := range domains { supportedTypes, err := metrics.SupportedDatabaseTypes(domain) if err != nil { return err } domainTypes = append(domainTypes, fmt.Sprintf("%s=%v", domain, supportedTypes)) + if len(supportedTypes) == 1 && supportedTypes[0] == blip.DatabaseTypeAny { + continue + } supported := map[blip.DatabaseType]bool{} for _, databaseType := range supportedTypes { supported[databaseType] = true - if i == 0 { + if !hasDatabaseConstraint { commonTypes[databaseType] = true } } - if i == 0 { + if !hasDatabaseConstraint { + hasDatabaseConstraint = true continue } for databaseType := range commonTypes { @@ -633,7 +638,7 @@ func validatePlanDatabaseCompatibility(plan blip.Plan) error { } } - if len(commonTypes) == 0 { + if hasDatabaseConstraint && len(commonTypes) == 0 { return fmt.Errorf("collectors have no common database type: %s", strings.Join(domainTypes, ", ")) } return nil diff --git a/plan/loader_test.go b/plan/loader_test.go index c0da20d..27706e0 100644 --- a/plan/loader_test.go +++ b/plan/loader_test.go @@ -19,6 +19,8 @@ import ( "github.com/cashapp/blip/v2/test/mock" ) +const externalType blip.DatabaseType = "test-database" + // -------------------------------------------------------------------------- func TestLoadDefault(t *testing.T) { @@ -178,7 +180,7 @@ func (f planDatabaseTypesFactory) DatabaseTypes(string) []blip.DatabaseType { func TestSharedPlansValidateDatabaseCompatibilityPerMonitor(t *testing.T) { const ( mysqlDomain = "test.mysql-plan" - postgresDomain = "test.postgres-plan" + externalDomain = "test.external-plan" sharedDomain = "test.shared-plan" ) factory := mock.MetricFactory{} @@ -186,17 +188,14 @@ func TestSharedPlansValidateDatabaseCompatibilityPerMonitor(t *testing.T) { t.Fatal(err) } t.Cleanup(func() { metrics.Remove(mysqlDomain) }) - if err := metrics.Register(postgresDomain, planDatabaseTypesFactory{ - databaseTypes: []blip.DatabaseType{blip.DatabaseTypePostgres}, + if err := metrics.Register(externalDomain, planDatabaseTypesFactory{ + databaseTypes: []blip.DatabaseType{externalType}, }); err != nil { t.Fatal(err) } - t.Cleanup(func() { metrics.Remove(postgresDomain) }) + t.Cleanup(func() { metrics.Remove(externalDomain) }) if err := metrics.Register(sharedDomain, planDatabaseTypesFactory{ - databaseTypes: []blip.DatabaseType{ - blip.DatabaseTypeMySQL, - blip.DatabaseTypePostgres, - }, + databaseTypes: []blip.DatabaseType{blip.DatabaseTypeAny}, }); err != nil { t.Fatal(err) } @@ -218,10 +217,10 @@ func TestSharedPlansValidateDatabaseCompatibilityPerMonitor(t *testing.T) { } } mysqlPlan := newPlan("mysql-plan", mysqlDomain) - postgresPlan := newPlan("postgres-plan", postgresDomain) + externalPlan := newPlan("external-plan", externalDomain) pl := plan.NewLoader(func(blip.ConfigPlans) ([]blip.Plan, error) { - return []blip.Plan{mysqlPlan, postgresPlan}, nil + return []blip.Plan{mysqlPlan, externalPlan}, nil }) if err := pl.LoadShared(blip.ConfigPlans{}, nil); err != nil { t.Fatalf("LoadShared: %v", err) @@ -232,50 +231,47 @@ func TestSharedPlansValidateDatabaseCompatibilityPerMonitor(t *testing.T) { t.Fatalf("LoadMonitor(mysql): %v", err) } if err := pl.LoadMonitor(blip.ConfigMonitor{ - MonitorId: "postgres", - DatabaseType: blip.DatabaseTypePostgres, + MonitorId: "external", + DatabaseType: externalType, }, nil); err != nil { - t.Fatalf("LoadMonitor(postgres): %v", err) + t.Fatalf("LoadMonitor(external): %v", err) } if _, err := pl.Plan("mysql", mysqlPlan.Name, nil); err != nil { t.Fatalf("MySQL plan for MySQL monitor: %v", err) } - if _, err := pl.Plan("postgres", postgresPlan.Name, nil); err != nil { - t.Fatalf("PostgreSQL plan for PostgreSQL monitor: %v", err) + if _, err := pl.Plan("external", externalPlan.Name, nil); err != nil { + t.Fatalf("external plan for external monitor: %v", err) } - if _, err := pl.Plan("mysql", postgresPlan.Name, nil); err == nil || - !strings.Contains(err.Error(), `collector test.postgres-plan does not support database type "mysql" (supported: [postgres])`) { - t.Fatalf("PostgreSQL plan for MySQL monitor error = %v", err) + if _, err := pl.Plan("mysql", externalPlan.Name, nil); err == nil || + !strings.Contains(err.Error(), `collector test.external-plan does not support database type "mysql" (supported: [test-database])`) { + t.Fatalf("external plan for MySQL monitor error = %v", err) } - if _, err := pl.Plan("postgres", mysqlPlan.Name, nil); err == nil || - !strings.Contains(err.Error(), `collector test.mysql-plan does not support database type "postgres" (supported: [mysql])`) { - t.Fatalf("MySQL plan for PostgreSQL monitor error = %v", err) + if _, err := pl.Plan("external", mysqlPlan.Name, nil); err == nil || + !strings.Contains(err.Error(), `collector test.mysql-plan does not support database type "test-database" (supported: [mysql])`) { + t.Fatalf("MySQL plan for external monitor error = %v", err) } } func TestValidatePlansRejectsCollectorsWithoutCommonDatabaseType(t *testing.T) { const ( mysqlDomain = "test.no-common-mysql" - postgresDomain = "test.no-common-postgres" + externalDomain = "test.no-common-external" sharedDomain = "test.no-common-shared" ) if err := metrics.Register(mysqlDomain, mock.MetricFactory{}); err != nil { t.Fatal(err) } t.Cleanup(func() { metrics.Remove(mysqlDomain) }) - if err := metrics.Register(postgresDomain, planDatabaseTypesFactory{ - databaseTypes: []blip.DatabaseType{blip.DatabaseTypePostgres}, + if err := metrics.Register(externalDomain, planDatabaseTypesFactory{ + databaseTypes: []blip.DatabaseType{externalType}, }); err != nil { t.Fatal(err) } - t.Cleanup(func() { metrics.Remove(postgresDomain) }) + t.Cleanup(func() { metrics.Remove(externalDomain) }) if err := metrics.Register(sharedDomain, planDatabaseTypesFactory{ - databaseTypes: []blip.DatabaseType{ - blip.DatabaseTypeMySQL, - blip.DatabaseTypePostgres, - }, + databaseTypes: []blip.DatabaseType{blip.DatabaseTypeAny}, }); err != nil { t.Fatal(err) } @@ -291,10 +287,10 @@ func TestValidatePlansRejectsCollectorsWithoutCommonDatabaseType(t *testing.T) { sharedDomain: {}, }, }, - "postgres": { + "external": { Freq: "5s", Collect: map[string]blip.Domain{ - postgresDomain: {}, + externalDomain: {}, }, }, }, @@ -302,13 +298,36 @@ func TestValidatePlansRejectsCollectorsWithoutCommonDatabaseType(t *testing.T) { err := plan.ValidatePlans([]blip.Plan{mixedPlan}) if err == nil { - t.Fatal("mixed MySQL and PostgreSQL plan is valid") + t.Fatal("mixed MySQL and external plan is valid") } expected := "collectors have no common database type: " + + "test.no-common-external=[test-database], " + "test.no-common-mysql=[mysql], " + - "test.no-common-postgres=[postgres], " + - "test.no-common-shared=[mysql postgres]" + "test.no-common-shared=[*]" if !strings.Contains(err.Error(), expected) { t.Fatalf("ValidatePlans error = %v", err) } } + +func TestValidatePlansAllowsDatabaseNeutralCollectors(t *testing.T) { + const domain = "test.database-neutral-plan" + if err := metrics.Register(domain, planDatabaseTypesFactory{ + databaseTypes: []blip.DatabaseType{blip.DatabaseTypeAny}, + }); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { metrics.Remove(domain) }) + + neutralPlan := blip.Plan{ + Name: "database-neutral-plan", + Levels: map[string]blip.Level{ + "level": { + Freq: "1s", + Collect: map[string]blip.Domain{domain: {}}, + }, + }, + } + if err := plan.ValidatePlans([]blip.Plan{neutralPlan}); err != nil { + t.Fatalf("database-neutral plan is invalid: %v", err) + } +} From 429b3f3e3efee23f777f0316810b4ec474dc2106 Mon Sep 17 00:00:00 2001 From: Ian Oberst Date: Fri, 7 Aug 2026 10:38:51 -0700 Subject: [PATCH 4/5] Clarify external database module docs Co-authored-by: Codex Ai-assisted: true --- docs/content/config/config-file.md | 48 ++++++---- docs/content/config/interpolation.md | 4 + docs/content/config/quick-ref.md | 12 ++- docs/content/develop/collectors.md | 2 + docs/content/develop/database-modules.md | 106 ++++++++++++----------- docs/content/develop/integration-api.md | 17 ++-- docs/content/metrics/reporting.md | 2 +- 7 files changed, 114 insertions(+), 77 deletions(-) diff --git a/docs/content/config/config-file.md b/docs/content/config/config-file.md index 1bb5d7d..eea7e10 100644 --- a/docs/content/config/config-file.md +++ b/docs/content/config/config-file.md @@ -210,7 +210,7 @@ For example, if Blip monitors Amazon RDS instances in region `us-east-1`, then s ### aws -The `aws` section configures Amazon RDS for MySQL. +The `aws` section configures Amazon RDS for MySQL. External database modules can also use its region and credential-source fields as documented by that module. ```yaml aws: @@ -264,7 +264,7 @@ See [Cloud / AWS / IAM Authentication]({{< ref "/cloud/aws#iam-authentication" > |**Valid values**|AWS Secrets Manager ARN| |**Default value**|| -The `password-secret` variable sets the AWS Secrets Manager ARN that contains the MySQL user password. +The `password-secret` variable sets the AWS Secrets Manager ARN that contains the database user password. When using the default parser, the secret JSON must contain a string `password` field, and it can optionally contain a string `username` field. Custom integrations can override this with [`Plugins.ParsePasswordSecret`]({{< ref "/develop/integration-api#aws-password-secrets" >}}). @@ -732,7 +732,7 @@ Do not verify the server address (MySQL hostname). ## Monitors -The `monitors` section is a list of MySQL instances to monitor. +The `monitors` section is a list of database targets. Blip's built-in binary monitors MySQL, so the examples below show MySQL instances. Each instance is a YAML dictionary containing any of the [monitor default sections](#monitor-defaults) with one exception: `mysql` variables are top-level in a monitor. The example below shows two different MySQL instances to monitor. @@ -761,9 +761,7 @@ Section [`exporter`](#exporter) is exactly the same in a monitor. Refer to [Monitor Defaults](#monitor-defaults) for configuring MySQL instances, and remember: [`mysql`](#mysql) variables are top-level in a monitor (omit `mysql:` and include the variables directly). -All monitors have three variables that only appear in monitor entries: `id`, -`meta`, and `plan`. A binary that enables an external database module can also -use `database-type` and `database-config`: +Monitor entries have three variables that do not appear in monitor defaults: `id`, `meta`, and `plan`. A binary that activates an [external database module]({{< ref "/develop/database-modules" >}}) can also use `database-type` and `database-config`: ```yaml monitors: @@ -774,15 +772,29 @@ monitors: module-option: value ``` -An omitted `database-type` retains Blip's built-in MySQL behavior. The type is -a literal module identifier, although direct environment-variable interpolation -such as `${DATABASE_TYPE}` is supported. Monitor-field interpolation is not -supported for this structural value. +### `database-type` -The contents of `database-config` belong to the selected module. Blip -interpolates string values, redacts the complete section from logged config, -and asks the registered module to validate it. Refer to the external module for -its supported fields. Blip itself does not register a non-MySQL module. +| | | +|-|-| +|**Type**|string| +|**Valid values**|`mysql` or a registered external database type| +|**Default value**|`mysql`| + +The `database-type` variable selects the database implementation for the monitor. An omitted value retains Blip's built-in MySQL behavior. A non-MySQL value requires the integrating binary to register the corresponding external module before `Server.Boot`. + +The value is a literal module identifier. Direct environment-variable interpolation such as `${DATABASE_TYPE}` is supported, but monitor-field interpolation is not supported because the type selects monitor defaults. + +### `database-config` + +| | | +|-|-| +|**Type**|key-value map| +|**Valid values**|module-specific| +|**Default value**|| + +The contents of `database-config` belong to the module selected by `database-type`. Blip applies environment and monitor variable interpolation recursively to string values, redacts the complete section from logged configuration, and asks the registered module to validate it. Refer to the external module for its supported fields. Blip itself does not register a non-MySQL module. + +External monitors do not inherit values from the top-level [`mysql`](#mysql) section or defaults for MySQL-only features. See [Database modules]({{< ref "/develop/database-modules" >}}) for the complete configuration contract. ### `id` @@ -792,14 +804,14 @@ its supported fields. Blip itself does not register a non-MySQL module. |**Valid values**|any string| |**Default value**|(automatic)| -The `id` variable uniquely identifies the MySQL instance in Blip. +The `id` variable uniquely identifies the monitor in Blip. Every monitor has a unique ID that, by default, Blip sets automatically. You can set monitor IDs manually, but it's better to let Blip set them automatically to avoid duplicates (which causes a fatal error). -Blip uses monitor IDs to track and report each MySQL instance in its own output and API. +Blip uses monitor IDs to track and report each monitor in its own output and API. -Blip does _not_ use monitor IDs to identify MySQL instances for reporting metrics, but you can use them if you want. +Blip does _not_ use monitor IDs to identify database targets for reporting metrics, but you can use them if you want. For example: ```yaml monitors: @@ -812,7 +824,7 @@ Since tags are passed to sinks (which report metrics), all sinks will receive th (Sinks receive the monitor ID at the code-level too, so technically this example is not necessary.) Monitor IDs are not guaranteed to be stable—they might change between Blip versions. -Therefore, do not rely on them outside of Blip for truly stable, unique MySQL instance identification. +Therefore, do not rely on them outside of Blip for stable, unique database target identification. ### `meta` diff --git a/docs/content/config/interpolation.md b/docs/content/config/interpolation.md index 2091874..2a44958 100644 --- a/docs/content/config/interpolation.md +++ b/docs/content/config/interpolation.md @@ -16,6 +16,10 @@ Monitor variable `${}` and `%{}` are always required. {{< /hint >}} +{{< hint type=note >}} +An external monitor's [`database-type`]({{< ref "config-file#database-type" >}}) supports environment-variable interpolation but not monitor-variable interpolation because it selects monitor defaults. String values nested inside [`database-config`]({{< ref "config-file#database-config" >}}) support both forms recursively. +{{< /hint >}} + Environment variable interpolation is a simple implementation of the shell standard. In Blip, only the two cases shown above are supported, and `default` must be a literal value (it cannot be another `${}`). diff --git a/docs/content/config/quick-ref.md b/docs/content/config/quick-ref.md index d2b1392..8550249 100644 --- a/docs/content/config/quick-ref.md +++ b/docs/content/config/quick-ref.md @@ -122,10 +122,11 @@ tls: skip-verify: false # --------------------------------------------------------------------------- -# Monitors (MySQL instances) +# Monitors # --------------------------------------------------------------------------- monitors: + # Built-in MySQL monitor - id: host1 # Optional; Blip auto-sets based on MySQL config # ----------------------------------------------- @@ -158,4 +159,13 @@ monitors: tags: hostname: "host1" # overrides monitor default foo: "bar" # new tag + + # ------------------------------------------------------------------------- + # External database module monitor (only when registered before server boot) + - id: external + database-type: my-database + hostname: database.example:1234 + username: metrics + database-config: + module-option: value ``` diff --git a/docs/content/develop/collectors.md b/docs/content/develop/collectors.md index 7bc2999..2483886 100644 --- a/docs/content/develop/collectors.md +++ b/docs/content/develop/collectors.md @@ -141,6 +141,8 @@ A collector that does not depend on the monitor's database engine returns `blip.DatabaseTypeAny`. The database-neutral type cannot be combined with specific database types. +When adding collectors for a complete external database engine, register that database type through a [database module]({{< ref "/develop/database-modules" >}}) before loading monitors. + ## Long-running As of Blip v1.2.0, long-running collectors are possible using one of two approaches: diff --git a/docs/content/develop/database-modules.md b/docs/content/develop/database-modules.md index 9024bb4..62467ed 100644 --- a/docs/content/develop/database-modules.md +++ b/docs/content/develop/database-modules.md @@ -1,31 +1,53 @@ --- --- -Blip is purpose-built for MySQL. An external database module can reuse its -monitor, plan, collection, transformation, and sink runtime without adding that -database implementation to Blip itself. +Blip is purpose-built for MySQL. An external database module can reuse its monitor, plan, collection, transformation, and sink runtime without adding that database implementation to Blip itself. {{< toc >}} -## Enable Before Boot +## Enable before boot -An external module is enabled by the integrating binary before `server.Boot`. -The module must: +The integrating binary must activate an external module before `Server.Boot`. A module should provide one entry point. It must: 1. Register its database type with `blip.RegisterDatabaseModule`. 2. Register its collectors through the existing `metrics.Register` API. 3. Decorate `Factories.DbConn` with its connection factory. -The module's connection factory should handle only its own database type and -delegate every other type to the previous factory. It must also preserve -`DbProviderFactory` delegation when the previous factory implements that -optional capability. This convention allows multiple external modules to -compose without changing Blip's built-in MySQL factory. +The integrating binary passes the values returned by `server.Defaults` to the module before boot: + +```go +env, plugins, factories := server.Defaults() +if err := mymodule.Enable(plugins, &factories); err != nil { + log.Fatal(err) +} + +s := server.Server{} +if err := s.Boot(env, plugins, factories); err != nil { + log.Fatal(err) +} +``` + +`server.Defaults` leaves `Factories.DbConn` unset because `Server.Boot` normally constructs Blip's built-in MySQL factory after loading configuration. A module that decorates the factory before boot must preserve that behavior. If `Factories.DbConn` is nil, construct the default MySQL factory with the configured AWS factory, `ModifyDB` plugin, and password-secret parser before wrapping it: + +```go +fallback := factories.DbConn +if fallback == nil { + fallback = dbconn.NewConnFactory( + factories.AWSConfig, + plugins.ModifyDB, + dbconn.WithPasswordSecretParser(plugins.ParsePasswordSecret), + ) +} +factories.DbConn = NewDatabaseFactory(fallback, moduleFactory) +``` + +The module's connection factory handles only its own database type and delegates every other type to the previous factory. If the decorator implements `DbProviderFactory`, its fallback path must call the previous factory's `MakeProvider` when available. Otherwise, it must call the previous `Make` method and wrap that connection in a single-pool provider whose `Close` method closes the connection. This convention preserves legacy factories and allows multiple external modules to compose in any activation order. + +If module activation stops after registering the database type or any collectors, remove those registrations before returning the error. This rollback allows a corrected activation attempt in the same process. ## Configuration -Every external monitor sets a literal `database-type` and can provide an opaque -`database-config` map: +Every external monitor sets [`database-type`]({{< ref "/config/config-file#database-type" >}}) and can provide an opaque [`database-config`]({{< ref "/config/config-file#database-config" >}}) map: ```yaml monitors: @@ -36,42 +58,22 @@ monitors: module-option: value ``` -An omitted database type remains MySQL. Direct `${ENV_VAR}` interpolation is -supported in `database-type`; monitor-field interpolation is intentionally not -supported because the type selects defaults before the rest of monitor -initialization. - -Blip recursively interpolates string values inside `database-config` and -redacts the entire opaque map when logging monitor configuration. A module uses -`blip.DecodeDatabaseConfig` to strictly decode the map into its own typed -configuration, then applies and validates its defaults in module code. Its -`DatabaseModule.ValidateConfig` implementation provides early validation during -monitor loading. - -MySQL socket, `my.cnf`, heartbeat, plan changing, plan-table storage, and -`mysqld_exporter` emulation are rejected for external database monitors. - -## Connections and Credentials - -`DbProviderFactory` is optional. A module that needs multiple connection pools -returns a `DbProvider`; Blip uses `Primary` for ordinary collectors and closes -the provider only after monitor subsystems and collectors stop. A specialized -collector factory implements `CollectorFactoryWithDBProvider` and type-asserts -the generic provider to a module-owned extension interface. - -The shared `credentials.Factory` supports IAM, Secrets Manager, password files, -static passwords, and passwordless authentication. Its `Dynamic` method takes -the engine's default port explicitly. The module remains responsible for -endpoint normalization, credential caching and refresh, authentication-error -classification, and connection retry behavior. - -## Collector Compatibility - -A module collector implements `CollectorFactoryDatabaseTypes` and returns its -database type. A collector that is independent of the monitor database returns -`DatabaseTypeAny`. A factory that does not implement the optional interface -retains Blip's historical MySQL compatibility. - -Blip validates that database-specific collectors in one plan have a common -database type, and it validates the selected plan against each monitor before -collector preparation. Database-neutral collectors do not constrain the plan. +An omitted database type remains MySQL. `database-type` supports direct `${ENV_VAR}` interpolation but not monitor-field interpolation because it selects defaults before the rest of monitor initialization. + +Blip recursively interpolates string values inside `database-config` and redacts the entire opaque map when logging monitor configuration. A module uses `blip.DecodeDatabaseConfig` to strictly decode the map into its own typed configuration, then applies and validates its defaults in module code. + +`DatabaseModule.ValidateConfig` receives monitor configuration by value. It can report configuration problems during monitor loading, but it cannot persist resolved defaults in Blip's opaque map. Use the same decode, default, and validation function from both `ValidateConfig` and the module's connection factory. + +External monitors do not inherit the top-level `mysql` defaults or defaults for the MySQL-only exporter, heartbeat, and plan-changing features. They can inherit shared AWS, TLS, tag, sink, and plan-file settings. MySQL socket, `my.cnf`, heartbeat, plan changing, plan-table storage, and `mysqld_exporter` emulation are rejected when configured on an external monitor. + +## Connections and credentials + +`DbProviderFactory` is optional. A module that needs multiple connection pools returns a `DbProvider`; Blip uses `Primary` for ordinary collectors and closes the provider only after monitor subsystems and collectors stop. A specialized collector factory implements `CollectorFactoryWithDBProvider` and type-asserts the generic provider to a module-owned extension interface. + +The shared `credentials.Factory` supports IAM, Secrets Manager, password files, static passwords, and passwordless authentication. Its `Dynamic` method takes the engine's default port explicitly. The module remains responsible for endpoint normalization, credential caching and refresh, authentication-error classification, and connection retry behavior. + +## Collector compatibility + +A module collector implements `CollectorFactoryDatabaseTypes` and returns its database type. A collector that is independent of the monitor database returns `DatabaseTypeAny`. A factory that does not implement the optional interface retains Blip's historical MySQL compatibility. + +Blip validates that database-specific collectors in one plan have a common database type, and it validates the selected plan against each monitor before collector preparation. Database-neutral collectors do not constrain the plan. diff --git a/docs/content/develop/integration-api.md b/docs/content/develop/integration-api.md index 40b6ea6..df833fe 100644 --- a/docs/content/develop/integration-api.md +++ b/docs/content/develop/integration-api.md @@ -2,7 +2,7 @@ --- Integration allows you to customize every major aspect of Blip without modifying its core code. -That makes it easy and safe to tailor Blip to meet any requirements and work in any environment. +That lets you tailor Blip to meet your requirements and work in your environment. For example, Blip does not collect [MySQL NDB](https://dev.mysql.com/doc/refman/en/mysql-cluster.html) metrics, but if you run NDB, you can write a [custom metrics collector]({{< ref "develop/collectors" >}}) for NDB, register it in Blip, then collect NDB metrics exactly the same as the built-in metric collectors. In fact, the built-in metric collectors implement the same interface; the only difference is that Blip automatically registers them on startup. @@ -19,6 +19,7 @@ How you integrate with Blip depends on what you're trying to customize: |Parsing AWS password secrets|Plugins| |AWS configs|Factories| |Database connections|Factories| +|External database engines|[Database modules]({{< ref "/develop/database-modules" >}})| |HTTP clients|Factories| |Timeouts|Variables| @@ -29,7 +30,7 @@ How you integrate with Blip depends on what you're trying to customize: ## Registry A registry maps a resource name to a factory that produces an object for the resource. -Blip has three registries: +Blip has three object registries: |Registry|Resource Name|Factory Produces| |:-------|:------------|:---------------| @@ -43,29 +44,35 @@ Every registry has a corresponding `Make` function that Blip uses to make object For example, when a [plan]({{< ref "intro/plans" >}}) collects the `status.global` domain, internally Blip makes a call like: ```go -collector, err := metrics.Make("status.global") +collector, err := metrics.Make("status.global", args) ``` That works because Blip registered the built-in factory for the `status.global` metric domain on startup. This is also how [custom metric collectors]({{< ref "develop/collectors" >}}) work: by registering a custom metric domain name and factory. +External database modules use a separate [`RegisterDatabaseModule`](https://pkg.go.dev/github.com/cashapp/blip/v2#RegisterDatabaseModule) hook to declare and validate a database type. The module still uses the metrics registry for its collectors. See [Database modules]({{< ref "/develop/database-modules" >}}) for the complete integration contract. + ## Factories [Factories](https://pkg.go.dev/github.com/cashapp/blip/v2#Factories) are interfaces that let you override certain object creation of Blip. Every factory is optional: if specified, it overrides the built-in factory. +An external database module decorates the database factory instead of replacing unrelated database support. See [Database modules]({{< ref "/develop/database-modules" >}}) for the fallback and provider delegation requirements. + ## Plugins [Plugins](https://pkg.go.dev/github.com/cashapp/blip/v2#Plugins) are function callbacks that let you override specific functionality of Blip. Every plugin is optional: if specified, it overrides the built-in functionality. -### AWS Password Secrets +### AWS password secrets -Set [`Plugins.ParsePasswordSecret`](https://pkg.go.dev/github.com/cashapp/blip/v2#Plugins) to customize how Blip maps the raw AWS Secrets Manager payload from [`config.aws.password-secret`]({{< ref "/config/config-file#password-secret" >}}) to MySQL credentials. +Set [`Plugins.ParsePasswordSecret`](https://pkg.go.dev/github.com/cashapp/blip/v2#Plugins) to customize how Blip maps the raw AWS Secrets Manager payload from [`config.aws.password-secret`]({{< ref "/config/config-file#password-secret" >}}) to database credentials. If this callback is not set, Blip uses [`DefaultPasswordSecretParser`](https://pkg.go.dev/github.com/cashapp/blip/v2#DefaultPasswordSecretParser): `password` is required, and `username` is optional. Blip passes `SecretString` bytes when present; otherwise, it passes `SecretBinary` bytes. The `credentials` argument is initialized with the configured monitor username; custom parsers must set `credentials.Password` and can override `credentials.Username`. +Blip's built-in MySQL factory and external modules that use the shared credential factory honor this callback. + ```go plugins.ParsePasswordSecret = func(ctx context.Context, cfg blip.ConfigMonitor, payload []byte, credentials *blip.DbCredentials) error { credentials.Password = string(payload) diff --git a/docs/content/metrics/reporting.md b/docs/content/metrics/reporting.md index 54e6206..b68320f 100644 --- a/docs/content/metrics/reporting.md +++ b/docs/content/metrics/reporting.md @@ -74,7 +74,7 @@ Internally, Blip stores metrics in a [`Metrics` data structure](https://pkg.go.d type Metrics struct { Begin time.Time // when collection started End time.Time // when collection completed - MonitorId string // ID of monitor (MySQL) + MonitorId string // ID of monitor Plan string // plan name Level string // level name State string // state of monitor From 8fcfa915adacac4d6e69bb425c6a4779d4fd41a8 Mon Sep 17 00:00:00 2001 From: Ian Oberst Date: Fri, 7 Aug 2026 11:01:41 -0700 Subject: [PATCH 5/5] Harden module configuration handling Co-authored-by: Codex Ai-assisted: true --- config.go | 54 +++++++++++++++++++++------------------- config_database_test.go | 23 +++++++++++++++++ monitor/monitor.go | 18 ++++++++++++-- monitor/provider_test.go | 13 +++++++++- 4 files changed, 80 insertions(+), 28 deletions(-) diff --git a/config.go b/config.go index 7df483e..c8faf92 100644 --- a/config.go +++ b/config.go @@ -32,15 +32,17 @@ func interpolateEnv(v string) string { if !strings.Contains(v, "${") { return v } - m := envvar.FindStringSubmatch(v) - if len(m) != 4 { - return v // strict match only - } - v2 := os.Getenv(m[1]) - if v2 == "" && m[2] != "" { - return m[3] - } - return envvar.ReplaceAllLiteralString(v, v2) + return envvar.ReplaceAllStringFunc(v, func(match string) string { + m := envvar.FindStringSubmatch(match) + if len(m) != 4 { + return match // strict match only + } + value := os.Getenv(m[1]) + if value == "" && m[2] != "" { + return m[3] + } + return value + }) } // setBool sets c to the value of b if c is nil (not set). Pointers are required @@ -641,25 +643,27 @@ func (c *ConfigMonitor) interpolateMon(v string) string { if !strings.Contains(v, "%{monitor.") { return v } - m := monvar.FindStringSubmatch(v) - if len(m) != 3 { - return v // strict match only - } - if strings.HasPrefix(m[2], "tags.") { - if c.Tags == nil { - return "" + return monvar.ReplaceAllStringFunc(v, func(match string) string { + m := monvar.FindStringSubmatch(match) + if len(m) != 3 || m[1] != "monitor" { + return match // strict match only } - s := strings.SplitN(m[2], ".", 2) - return c.Tags[s[1]] - } else if strings.HasPrefix(m[2], "meta.") { - if c.Meta == nil { - return "" + if strings.HasPrefix(m[2], "tags.") { + if c.Tags == nil { + return "" + } + s := strings.SplitN(m[2], ".", 2) + return c.Tags[s[1]] + } else if strings.HasPrefix(m[2], "meta.") { + if c.Meta == nil { + return "" + } + s := strings.SplitN(m[2], ".", 2) + return c.Meta[s[1]] } - s := strings.SplitN(m[2], ".", 2) - return c.Meta[s[1]] - } - return monvar.ReplaceAllString(v, c.fieldValue(m[2])) + return c.fieldValue(m[2]) + }) } func (c *ConfigMonitor) fieldValue(f string) string { diff --git a/config_database_test.go b/config_database_test.go index 367f9cb..eccda5c 100644 --- a/config_database_test.go +++ b/config_database_test.go @@ -112,6 +112,29 @@ func TestConfigMonitorExternalModuleValidationAndInterpolation(t *testing.T) { } } +func TestConfigMonitorExternalModuleInterpolationResolvesEachPlaceholder(t *testing.T) { + t.Setenv("BLIP_TEST_DATABASE_HOST", "database.example") + t.Setenv("BLIP_TEST_DATABASE_PORT", "5432") + + monitor := blip.ConfigMonitor{ + MonitorId: "external-monitor", + Hostname: "database.example:5432", + DatabaseConfig: blip.ConfigDatabase{ + "endpoint": "${BLIP_TEST_DATABASE_HOST}:${BLIP_TEST_DATABASE_PORT}", + "identity": "%{monitor.id}@%{monitor.hostname}", + }, + } + monitor.InterpolateEnvVars() + monitor.InterpolateMonitor() + + if got := monitor.DatabaseConfig["endpoint"]; got != "database.example:5432" { + t.Fatalf("endpoint = %q, expected database.example:5432", got) + } + if got := monitor.DatabaseConfig["identity"]; got != "external-monitor@database.example:5432" { + t.Fatalf("identity = %q, expected external-monitor@database.example:5432", got) + } +} + func TestConfigMonitorExternalModuleInterpolationPreservesTypedContainers(t *testing.T) { const databaseType blip.DatabaseType = "test-typed-interpolation" t.Setenv("BLIP_TEST_TYPED_VALUE", "from-environment") diff --git a/monitor/monitor.go b/monitor/monitor.go index 022fc45..57f77ba 100644 --- a/monitor/monitor.go +++ b/monitor/monitor.go @@ -10,6 +10,7 @@ package monitor import ( "database/sql" "fmt" + "reflect" "runtime" "sync" "time" @@ -485,12 +486,12 @@ func (m *Monitor) makeDB() (blip.DbProvider, *sql.DB, string, error) { provider, dsn, err := providerFactory.MakeProvider(m.cfg) if err != nil { - if provider != nil { + if !nilDBProvider(provider) { provider.Close() } return nil, nil, "", err } - if provider == nil { + if nilDBProvider(provider) { return nil, nil, "", fmt.Errorf("database provider factory returned a nil provider") } db := provider.Primary() @@ -501,6 +502,19 @@ func (m *Monitor) makeDB() (blip.DbProvider, *sql.DB, string, error) { return provider, db, dsn, nil } +func nilDBProvider(provider blip.DbProvider) bool { + if provider == nil { + return true + } + value := reflect.ValueOf(provider) + switch value.Kind() { + case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Ptr, reflect.Slice: + return value.IsNil() + default: + return false + } +} + // closeDB releases the current monitor-owned database resources. The caller // must hold runMux. func (m *Monitor) closeDB() { diff --git a/monitor/provider_test.go b/monitor/provider_test.go index 6bee7ad..09d433a 100644 --- a/monitor/provider_test.go +++ b/monitor/provider_test.go @@ -88,6 +88,17 @@ func TestMonitorMakeDBRejectsInvalidProvider(t *testing.T) { name: "nil provider", wantError: "nil provider", }, + { + name: "typed nil provider", + provider: (*testDBProvider)(nil), + wantError: "nil provider", + }, + { + name: "factory error with typed nil provider", + provider: (*testDBProvider)(nil), + factoryErr: errors.New("make failed"), + wantError: "make failed", + }, { name: "nil primary", provider: &testDBProvider{}, @@ -110,7 +121,7 @@ func TestMonitorMakeDBRejectsInvalidProvider(t *testing.T) { if err == nil || !errors.Is(err, test.factoryErr) && !strings.Contains(err.Error(), test.wantError) { t.Fatalf("makeDB error = %v, expected %q", err, test.wantError) } - if provider, ok := test.provider.(*testDBProvider); ok && provider.closeCalls != 1 { + if provider, ok := test.provider.(*testDBProvider); ok && provider != nil && provider.closeCalls != 1 { t.Fatalf("provider Close calls = %d, expected 1", provider.closeCalls) } })