Skip to content

feat: add prometheus metrics to database.Store #7713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
May 31, 2023
Merged

feat: add prometheus metrics to database.Store #7713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6c73f9d
feat: add prometheus metrics to database.Store
johnstcn May 30, 2023
c0ea9ef
ensure wrapping
johnstcn May 30, 2023
b153939
only wrap if we have a prometheus registry; add safety check for non-dev
johnstcn May 31, 2023
6b867c6
measure tx duration
johnstcn May 31, 2023
5385657
Merge remote-tracking branch 'origin/main' into cj/db-prom
johnstcn May 31, 2023
d3cf018
fixup! Merge remote-tracking branch 'origin/main' into cj/db-prom
johnstcn May 31, 2023
0ecd6aa
chore: fix flaky TestLicensesListFake/Mainline
johnstcn May 31, 2023
b846d0b
fixup! chore: fix flaky TestLicensesListFake/Mainline
johnstcn May 31, 2023
0b9e836
fix double-wrapping for good
johnstcn May 31, 2023
9aee243
fixup! fix double-wrapping for good
johnstcn May 31, 2023
9367f6a
slices.Contains
johnstcn May 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions cli/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import (
"github.com/coder/coder/coderd/autobuild/executor"
"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/database/dbfake"
"github.com/coder/coder/coderd/database/dbmetrics"
"github.com/coder/coder/coderd/database/dbpurge"
"github.com/coder/coder/coderd/database/migrations"
"github.com/coder/coder/coderd/devtunnel"
Expand Down Expand Up @@ -586,7 +587,8 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
}

if cfg.InMemoryDatabase {
options.Database = dbfake.New()
// This is only used for testing.
options.Database = dbmetrics.New(dbfake.New(), options.PrometheusRegistry)
options.Pubsub = database.NewPubsubInMemory()
} else {
sqlDB, err := connectToPostgres(ctx, logger, sqlDriver, cfg.PostgresURL.String())
Expand All @@ -597,7 +599,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
_ = sqlDB.Close()
}()

options.Database = database.New(sqlDB)
options.Database = dbmetrics.New(database.New(sqlDB), options.PrometheusRegistry)
options.Pubsub, err = database.NewPubsub(ctx, sqlDB, cfg.PostgresURL.String())
if err != nil {
return xerrors.Errorf("create pubsub: %w", err)
Expand Down
10 changes: 10 additions & 0 deletions coderd/coderd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
"github.com/coder/coder/coderd/awsidentity"
"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/database/dbauthz"
"github.com/coder/coder/coderd/database/dbmetrics"
"github.com/coder/coder/coderd/database/dbtype"
"github.com/coder/coder/coderd/gitauth"
"github.com/coder/coder/coderd/gitsshkey"
Expand Down Expand Up @@ -176,6 +177,11 @@ func New(options *Options) *API {
options = &Options{}
}

// Safety check: if we're not running a unit test, we *must* have a Prometheus registry.
if options.PrometheusRegistry == nil && flag.Lookup("test.v") == nil {
panic("developer error: options.PrometheusRegistry is nil and not running a unit test")
}

Comment on lines +180 to +184
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: I'm open to removing this, but I think it's a good check to have. We don't want to accidentally publish a release with no prometheus metrics.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we always do this? Seems wasteful if --prometheus-enable=false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we do. It is a little wasteful, but less error-prone than having to check if the prometheus registerer is nil everywhere.

if options.DeploymentValues.DisableOwnerWorkspaceExec {
rbac.ReloadBuiltinRoles(&rbac.RoleOptions{
NoOwnerWorkspaceExec: true,
Expand All @@ -185,6 +191,10 @@ func New(options *Options) *API {
if options.Authorizer == nil {
options.Authorizer = rbac.NewCachingAuthorizer(options.PrometheusRegistry)
}
// The below are no-ops if already wrapped.
if options.PrometheusRegistry != nil {
options.Database = dbmetrics.New(options.Database, options.PrometheusRegistry)
}
options.Database = dbauthz.New(
options.Database,
options.Authorizer,
Expand Down
13 changes: 13 additions & 0 deletions coderd/database/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,20 @@ type Store interface {
querier
// customQuerier contains custom queries that are not generated.
customQuerier
// wrapper allows us to detect if the interface has been wrapped.
wrapper

Ping(ctx context.Context) (time.Duration, error)
InTx(func(Store) error, *sql.TxOptions) error
}

type wrapper interface {
// Wrappers returns a list of wrappers that have been applied to the store.
// This is used to detect if the store has already wrapped, and avoid
// double-wrapping.
Wrappers() []string
}

// DBTX represents a database connection or transaction.
type DBTX interface {
ExecContext(context.Context, string, ...interface{}) (sql.Result, error)
Expand Down Expand Up @@ -60,6 +69,10 @@ type sqlQuerier struct {
db DBTX
}

func (*sqlQuerier) Wrappers() []string {
return []string{}
}

// Ping returns the time it takes to ping the database.
func (q *sqlQuerier) Ping(ctx context.Context) (time.Duration, error) {
start := time.Now()
Expand Down
9 changes: 8 additions & 1 deletion coderd/database/dbauthz/dbauthz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"

"github.com/google/uuid"
"golang.org/x/exp/slices"
"golang.org/x/xerrors"

"github.com/open-policy-agent/opa/topdown"
Expand All @@ -17,6 +18,8 @@ import (

var _ database.Store = (*querier)(nil)

const wrapname = "dbauthz.querier"

// NoActorError wraps ErrNoRows for the api to return a 404. This is the correct
// response when the user is not authorized.
var NoActorError = xerrors.Errorf("no authorization actor in context: %w", sql.ErrNoRows)
Expand Down Expand Up @@ -89,7 +92,7 @@ type querier struct {
func New(db database.Store, authorizer rbac.Authorizer, logger slog.Logger) database.Store {
// If the underlying db store is already a querier, return it.
// Do not double wrap.
if _, ok := db.(*querier); ok {
if slices.Contains(db.Wrappers(), wrapname) {
return db
}
return &querier{
Expand All @@ -99,6 +102,10 @@ func New(db database.Store, authorizer rbac.Authorizer, logger slog.Logger) data
}
}

func (q *querier) Wrappers() []string {
return append(q.db.Wrappers(), wrapname)
}

// authorizeContext is a helper function to authorize an action on an object.
func (q *querier) authorizeContext(ctx context.Context, action rbac.Action, object rbac.Objecter) error {
act, ok := ActorFromContext(ctx)
Expand Down
2 changes: 1 addition & 1 deletion coderd/database/dbauthz/dbauthz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func TestDBAuthzRecursive(t *testing.T) {
for i := 2; i < method.Type.NumIn(); i++ {
ins = append(ins, reflect.New(method.Type.In(i)).Elem())
}
if method.Name == "InTx" || method.Name == "Ping" {
if method.Name == "InTx" || method.Name == "Ping" || method.Name == "Wrappers" {
continue
}
// Log the name of the last method, so if there is a panic, it is
Expand Down
13 changes: 10 additions & 3 deletions coderd/database/dbauthz/setup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"strings"
"testing"

"github.com/golang/mock/gomock"
"github.com/google/uuid"
"github.com/open-policy-agent/opa/topdown"
"github.com/stretchr/testify/require"
Expand All @@ -19,14 +20,16 @@ import (
"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/database/dbauthz"
"github.com/coder/coder/coderd/database/dbfake"
"github.com/coder/coder/coderd/database/dbmock"
"github.com/coder/coder/coderd/rbac"
"github.com/coder/coder/coderd/rbac/regosql"
"github.com/coder/coder/coderd/util/slice"
)

var skipMethods = map[string]string{
"InTx": "Not relevant",
"Ping": "Not relevant",
"InTx": "Not relevant",
"Ping": "Not relevant",
"Wrappers": "Not relevant",
}

// TestMethodTestSuite runs MethodTestSuite.
Expand All @@ -52,7 +55,11 @@ type MethodTestSuite struct {
// SetupSuite sets up the suite by creating a map of all methods on querier
// and setting their count to 0.
func (s *MethodTestSuite) SetupSuite() {
az := dbauthz.New(nil, nil, slog.Make())
ctrl := gomock.NewController(s.T())
mockStore := dbmock.NewMockStore(ctrl)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Emyrk I think it makes sense to use the mock here instead of nil, as it will allow us to immediately see if we accidentally call methods of the underlying database.Store.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would just panic if the db is set to nil, which would tell us a call was made. Does the mock panic?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see though, for the wrappers method.

// We intentionally set no expectations apart from this.
mockStore.EXPECT().Wrappers().Return([]string{}).AnyTimes()
az := dbauthz.New(mockStore, nil, slog.Make())
// Take the underlying type of the interface.
azt := reflect.TypeOf(az).Elem()
s.methodAccounting = make(map[string]int)
Expand Down
4 changes: 4 additions & 0 deletions coderd/database/dbfake/databasefake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ type fakeQuerier struct {
*data
}

func (*fakeQuerier) Wrappers() []string {
return []string{}
}

type fakeTx struct {
*fakeQuerier
locks map[int64]struct{}
Expand Down
Loading