From 38fc840184678f556ade78347c04e29638af1c70 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 8 Aug 2023 09:51:23 -0700 Subject: [PATCH] sa: refactor how metrics and logging are set up (#7031) This eliminates the need for a pair of accessors on `db.WrappedMap` that expose the underlying `*sql.DB` and `*borp.DbMap`. Fixes #6991 --- cmd/config.go | 15 ---------- cmd/expiration-mailer/main_test.go | 5 ++-- cmd/rocsp-tool/client_test.go | 1 - db/map.go | 8 ----- sa/database.go | 48 +++++++++++++++--------------- sa/sa.go | 2 -- 6 files changed, 27 insertions(+), 52 deletions(-) diff --git a/cmd/config.go b/cmd/config.go index fbe0c7276ac..62c99005a27 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -9,7 +9,6 @@ import ( "os" "strings" - "github.com/go-sql-driver/mysql" "github.com/prometheus/client_golang/prometheus" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" "google.golang.org/grpc/resolver" @@ -88,20 +87,6 @@ func (d *DBConfig) URL() (string, error) { return strings.TrimSpace(string(url)), err } -// DSNAddressAndUser returns the Address and User of the DBConnect DSN from -// this object. -func (d *DBConfig) DSNAddressAndUser() (string, string, error) { - dsnStr, err := d.URL() - if err != nil { - return "", "", fmt.Errorf("failed to load DBConnect URL: %s", err) - } - config, err := mysql.ParseDSN(dsnStr) - if err != nil { - return "", "", fmt.Errorf("failed to parse DSN from the DBConnect URL: %s", err) - } - return config.Addr, config.User, nil -} - type SMTPConfig struct { PasswordConfig Server string `validate:"required"` diff --git a/cmd/expiration-mailer/main_test.go b/cmd/expiration-mailer/main_test.go index 244eaf0b328..109d8c57f25 100644 --- a/cmd/expiration-mailer/main_test.go +++ b/cmd/expiration-mailer/main_test.go @@ -937,15 +937,16 @@ type testCtx struct { } func setup(t *testing.T, nagTimes []time.Duration) *testCtx { + log := blog.NewMock() + // We use the test_setup user (which has full permissions to everything) // because the SA we return is used for inserting data to set up the test. - dbMap, err := sa.DBMapForTest(vars.DBConnSAFullPerms) + dbMap, err := sa.DBMapForTestWithLog(vars.DBConnSAFullPerms, log) if err != nil { t.Fatalf("Couldn't connect the database: %s", err) } fc := clock.NewFake() - log := blog.NewMock() ssa, err := sa.NewSQLStorageAuthority(dbMap, dbMap, nil, 1, 0, fc, log, metrics.NoopRegisterer) if err != nil { t.Fatalf("unable to create SQLStorageAuthority: %s", err) diff --git a/cmd/rocsp-tool/client_test.go b/cmd/rocsp-tool/client_test.go index 9fffb6c0352..5b966d1ca90 100644 --- a/cmd/rocsp-tool/client_test.go +++ b/cmd/rocsp-tool/client_test.go @@ -56,7 +56,6 @@ func TestGetStartingID(t *testing.T) { dbMap, err := sa.DBMapForTest(vars.DBConnSAFullPerms) test.AssertNotError(t, err, "failed setting up db client") defer test.ResetBoulderTestDatabase(t)() - sa.SetSQLDebug(dbMap, blog.Get()) cs := core.CertificateStatus{ Serial: "1337", diff --git a/db/map.go b/db/map.go index 4de5fcabf91..4abd2dce502 100644 --- a/db/map.go +++ b/db/map.go @@ -68,14 +68,6 @@ func NewWrappedMap(dbMap *borp.DbMap) *WrappedMap { return &WrappedMap{dbMap: dbMap} } -func (m *WrappedMap) SQLDb() *sql.DB { - return m.dbMap.Db -} - -func (m *WrappedMap) BorpDB() *borp.DbMap { - return m.dbMap -} - func (m *WrappedMap) TableFor(t reflect.Type, checkPK bool) (*borp.TableMap, error) { return m.dbMap.TableFor(t, checkPK) } diff --git a/sa/database.go b/sa/database.go index 254059f0f0a..37b58fa473e 100644 --- a/sa/database.go +++ b/sa/database.go @@ -67,26 +67,11 @@ func InitWrappedDb(config cmd.DBConfig, scope prometheus.Registerer, logger blog return nil, err } - dbMap, err := newDbMapFromMySQLConfig(mysqlConfig, settings) + dbMap, err := newDbMapFromMySQLConfig(mysqlConfig, settings, scope, logger) if err != nil { return nil, err } - if logger != nil { - SetSQLDebug(dbMap, logger) - } - - addr, user, err := config.DSNAddressAndUser() - if err != nil { - return nil, fmt.Errorf("while parsing DSN: %w", err) - } - - if scope != nil { - err = initDBMetrics(dbMap.SQLDb(), scope, settings, addr, user) - if err != nil { - return nil, fmt.Errorf("while initializing metrics: %w", err) - } - } return dbMap, nil } @@ -95,6 +80,12 @@ func InitWrappedDb(config cmd.DBConfig, scope prometheus.Registerer, logger blog // tables. It automatically maps the tables for the primary parts of Boulder // around the Storage Authority. func DBMapForTest(dbConnect string) (*boulderDB.WrappedMap, error) { + return DBMapForTestWithLog(dbConnect, nil) +} + +// DBMapForTestWithLog does the same as DBMapForTest but also routes the debug logs +// from the database driver to the given log (usually a `blog.NewMock`). +func DBMapForTestWithLog(dbConnect string, log blog.Logger) (*boulderDB.WrappedMap, error) { var err error var config *mysql.Config @@ -103,7 +94,7 @@ func DBMapForTest(dbConnect string) (*boulderDB.WrappedMap, error) { return nil, err } - return newDbMapFromMySQLConfig(config, DbSettings{}) + return newDbMapFromMySQLConfig(config, DbSettings{}, nil, log) } // sqlOpen is used in the tests to check that the arguments are properly @@ -148,7 +139,10 @@ var setConnMaxIdleTime = func(db *sql.DB, connMaxIdleTime time.Duration) { // - pings the database (and errors if it's unreachable) // - wraps the connection in a borp.DbMap so we can use the handy Get/Insert methods borp provides // - wraps that in a db.WrappedMap to get more useful error messages -func newDbMapFromMySQLConfig(config *mysql.Config, settings DbSettings) (*boulderDB.WrappedMap, error) { +// +// If logger is non-nil, it will receive debug log messages from borp. +// If scope is non-nil, it will be used to register Prometheus metrics. +func newDbMapFromMySQLConfig(config *mysql.Config, settings DbSettings, scope prometheus.Registerer, logger blog.Logger) (*boulderDB.WrappedMap, error) { err := adjustMySQLConfig(config) if err != nil { return nil, err @@ -166,9 +160,20 @@ func newDbMapFromMySQLConfig(config *mysql.Config, settings DbSettings) (*boulde setConnMaxLifetime(db, settings.ConnMaxLifetime) setConnMaxIdleTime(db, settings.ConnMaxIdleTime) + if scope != nil { + err = initDBMetrics(db, scope, settings, config.Addr, config.User) + if err != nil { + return nil, fmt.Errorf("while initializing metrics: %w", err) + } + } + dialect := borp.MySQLDialect{Engine: "InnoDB", Encoding: "UTF8"} dbmap := &borp.DbMap{Db: db, Dialect: dialect, TypeConverter: BoulderTypeConverter{}} + if logger != nil { + dbmap.TraceOn("SQL: ", &SQLLogger{logger}) + } + initTables(dbmap) return boulderDB.NewWrappedMap(dbmap), nil } @@ -239,17 +244,12 @@ func adjustMySQLConfig(conf *mysql.Config) error { return nil } -// SetSQLDebug enables borp SQL-level Debugging -func SetSQLDebug(dbMap *boulderDB.WrappedMap, log blog.Logger) { - dbMap.BorpDB().TraceOn("SQL: ", &SQLLogger{log}) -} - // SQLLogger adapts the Boulder Logger to a format borp can use. type SQLLogger struct { blog.Logger } -// Printf adapts the AuditLogger to borp's interface +// Printf adapts the Logger to borp's interface func (log *SQLLogger) Printf(format string, v ...interface{}) { log.Debugf(format, v...) } diff --git a/sa/sa.go b/sa/sa.go index e8c990a2ccf..4851ec38a55 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -56,8 +56,6 @@ func NewSQLStorageAuthorityWrapping( dbMap *db.WrappedMap, stats prometheus.Registerer, ) (*SQLStorageAuthority, error) { - SetSQLDebug(dbMap, ssaro.log) - rateLimitWriteErrors := prometheus.NewCounter(prometheus.CounterOpts{ Name: "rate_limit_write_errors", Help: "number of failed ratelimit update transactions during AddCertificate",