feat: add prometheus metrics to database.Store #7713

johnstcn · 2023-05-30T13:08:50Z

This PR adds a dbmetrics package and wraps database.Store with a Prometheus HistogramVec of timings.

The interface is manually written for now; the implementation is simple enough to not really require the overhead of generation.

Update: to fix the issue of potentially double-wrapping the database.Store, added a Wrappers() method to allow us to detect what wrappers are in use.

Emyrk · 2023-05-30T13:15:07Z

I see value in this.

Only comment is to maybe wrap the db like we do with dbauthz:

coder/coderd/coderd.go

Lines 188 to 192 in 702c908

    
           options.Database = dbauthz.New( 
        
           	options.Database, 
        
           	options.Authorizer, 
        
           	options.Logger.Named("authz_querier"), 
        
           )

Make it so it's always wrapped.

coderd/coderd.go

coderd/database/dbmetrics/dbmetrics.go

cli/server.go

johnstcn · 2023-05-31T12:04:38Z

Make it so it's always wrapped.

In order to fix this properly, I ended up adding a Wrappers() []string method on database.Store so that we can avoid the case where

We wrap with A
We wrap with B
We wrap again with A

johnstcn · 2023-05-31T12:05:47Z

coderd/coderd.go

+	// 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")
+	}
+


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.

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

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

johnstcn · 2023-05-31T12:06:51Z

coderd/database/dbauthz/setup_test.go

@@ -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)


@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.

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

Ah I see though, for the wrappers method.

johnstcn · 2023-05-31T12:07:47Z

enterprise/cli/licenses_test.go

@@ -158,7 +159,13 @@ func TestLicensesListFake(t *testing.T) {
 		assert.Equal(t, "claim1", licenses[0].Claims["h1"])
 		assert.Equal(t, int32(5), licenses[1].ID)
 		assert.Equal(t, "claim2", licenses[1].Claims["h2"])
-		assert.Equal(t, "2024-04-06T16:53:35Z", licenses[0].Claims["license_expires"])


review: this unit test breaks if your timezone is different 😛

Is this an underlying bug we should fix instead or working as expected?

I think showing the expiry in the user's local timezone is probably fine.
EDIT: we used to show the expiry as a UNIX timestamp; it was changed to show RFC3339 in #7687. I think showing with local timezone is in line with this change.

mafredri · 2023-05-31T12:25:27Z

coderd/coderd.go

+	// 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")
+	}
+


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

coderd/database/dbauthz/dbauthz.go

mafredri · 2023-05-31T12:30:32Z

enterprise/cli/licenses_test.go

@@ -158,7 +159,13 @@ func TestLicensesListFake(t *testing.T) {
 		assert.Equal(t, "claim1", licenses[0].Claims["h1"])
 		assert.Equal(t, int32(5), licenses[1].ID)
 		assert.Equal(t, "claim2", licenses[1].Claims["h2"])
-		assert.Equal(t, "2024-04-06T16:53:35Z", licenses[0].Claims["license_expires"])


Is this an underlying bug we should fix instead or working as expected?

Emyrk · 2023-05-31T12:42:35Z

I was thinking about that double wrapping yesterday. Can we use the same style as errors and just implement an "unwrap" method? Then we can recursively unwrap to check it it's wrapped with something

mafredri · 2023-05-31T12:54:01Z

I was thinking about that double wrapping yesterday. Can we use the same style as errors and just implement an "unwrap" method? Then we can recursively unwrap to check it it's wrapped with something

I thought about this too, and both approaches are solid IMO. It's perhaps a philosophical question, do we want to enable the ability to unwrap, e.g. escaping dbauthz?

johnstcn · 2023-05-31T12:58:07Z

I was thinking about that double wrapping yesterday. Can we use the same style as errors and just implement an "unwrap" method? Then we can recursively unwrap to check it it's wrapped with something

I thought about this too, and both approaches are solid IMO. It's perhaps a philosophical question, do we want to enable the ability to unwrap, e.g. escaping dbauthz?

I'd be against it for exactly that reason! 🙃

Emyrk · 2023-05-31T13:14:44Z

Hmm good point. For unit testing unwrapping could be helpful. But I understand why you choose against it.

johnstcn · 2023-05-31T13:15:52Z

Hmm good point. For unit testing unwrapping could be helpful. But I understand why you choose against it.

True, but in a unit test ideally you'd just pass in the database.Store you want with or without the wrappers you need.

Emyrk · 2023-05-31T13:17:44Z

coderd/database/dbauthz/setup_test.go

@@ -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)


Ah I see though, for the wrappers method.

feat: add prometheus metrics to database.Store

6c73f9d

johnstcn self-assigned this May 30, 2023

ensure wrapping

c0ea9ef

johnstcn marked this pull request as ready for review May 30, 2023 13:28

johnstcn requested review from mafredri, Emyrk and mtojek May 30, 2023 13:28

johnstcn commented May 30, 2023

View reviewed changes

coderd/coderd.go Outdated Show resolved Hide resolved

mafredri reviewed May 30, 2023

View reviewed changes

coderd/database/dbmetrics/dbmetrics.go Outdated Show resolved Hide resolved

cli/server.go Show resolved Hide resolved

johnstcn added 8 commits May 31, 2023 10:28

only wrap if we have a prometheus registry; add safety check for non-dev

b153939

measure tx duration

6b867c6

Merge remote-tracking branch 'origin/main' into cj/db-prom

5385657

fixup! Merge remote-tracking branch 'origin/main' into cj/db-prom

d3cf018

chore: fix flaky TestLicensesListFake/Mainline

0ecd6aa

fixup! chore: fix flaky TestLicensesListFake/Mainline

b846d0b

fix double-wrapping for good

0b9e836

fixup! fix double-wrapping for good

9aee243

johnstcn requested a review from mafredri May 31, 2023 12:04

johnstcn commented May 31, 2023

View reviewed changes

mafredri approved these changes May 31, 2023

View reviewed changes

Emyrk approved these changes May 31, 2023

View reviewed changes

slices.Contains

9367f6a

johnstcn merged commit 784696d into main May 31, 2023

johnstcn deleted the cj/db-prom branch May 31, 2023 13:55

github-actions bot locked and limited conversation to collaborators May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add prometheus metrics to database.Store #7713

feat: add prometheus metrics to database.Store #7713

johnstcn commented May 30, 2023 •

edited

Loading

Emyrk commented May 30, 2023

johnstcn commented May 31, 2023

johnstcn May 31, 2023

mafredri May 31, 2023

johnstcn May 31, 2023

johnstcn May 31, 2023

Emyrk May 31, 2023

Emyrk May 31, 2023

johnstcn May 31, 2023

mafredri May 31, 2023

johnstcn May 31, 2023 •

edited

Loading

mafredri May 31, 2023

mafredri May 31, 2023

Emyrk commented May 31, 2023

mafredri commented May 31, 2023

johnstcn commented May 31, 2023

Emyrk commented May 31, 2023

johnstcn commented May 31, 2023

Emyrk May 31, 2023

feat: add prometheus metrics to database.Store #7713

feat: add prometheus metrics to database.Store #7713

Conversation

johnstcn commented May 30, 2023 • edited Loading

Emyrk commented May 30, 2023

johnstcn commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emyrk commented May 31, 2023

mafredri commented May 31, 2023

johnstcn commented May 31, 2023

Emyrk commented May 31, 2023

johnstcn commented May 31, 2023

Choose a reason for hiding this comment

johnstcn commented May 30, 2023 •

edited

Loading

johnstcn May 31, 2023 •

edited

Loading