Skip to content

Commit

Permalink
sa: refactor how metrics and logging are set up (#7031)
Browse files Browse the repository at this point in the history
This eliminates the need for a pair of accessors on `db.WrappedMap` that
expose the underlying `*sql.DB` and `*borp.DbMap`.

Fixes #6991
  • Loading branch information
jsha authored Aug 8, 2023
1 parent 9a4f0ca commit 38fc840
Show file tree
Hide file tree
Showing 6 changed files with 27 additions and 52 deletions.
15 changes: 0 additions & 15 deletions cmd/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"`
Expand Down
5 changes: 3 additions & 2 deletions cmd/expiration-mailer/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 0 additions & 1 deletion cmd/rocsp-tool/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
8 changes: 0 additions & 8 deletions db/map.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
48 changes: 24 additions & 24 deletions sa/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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...)
}
Expand Down
2 changes: 0 additions & 2 deletions sa/sa.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit 38fc840

Please sign in to comment.