- Notifications
You must be signed in to change notification settings - Fork1k
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I see value in this. Only comment is to maybe wrap the db like we do with dbauthz: Lines 188 to 192 in702c908
Make it so it's always wrapped. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
In order to fix this properly, I ended up adding a
|
| // Safety check: if we're not running a unit test, we *must* have a Prometheus registry. | ||
| ifoptions.PrometheusRegistry==nil&&flag.Lookup("test.v")==nil { | ||
| panic("developer error: options.PrometheusRegistry is nil and not running a unit test") | ||
| } | ||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| func (s*MethodTestSuite)SetupSuite() { | ||
| az:=dbauthz.New(nil,nil,slog.Make()) | ||
| ctrl:=gomock.NewController(s.T()) | ||
| mockStore:=dbmock.NewMockStore(ctrl) |
There was a problem hiding this comment.
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 ofnil, as it will allow us to immediately see if we accidentally call methods of the underlyingdatabase.Store.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: this unit test breaks if your timezone is different 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this an underlying bug we should fix instead or working as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| // Safety check: if we're not running a unit test, we *must* have a Prometheus registry. | ||
| ifoptions.PrometheusRegistry==nil&&flag.Lookup("test.v")==nil { | ||
| panic("developer error: options.PrometheusRegistry is nil and not running a unit test") | ||
| } | ||
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading.Please reload this page.
| 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"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this an underlying bug we should fix instead or working as expected?
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! 🙃 |
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 |
| func (s*MethodTestSuite)SetupSuite() { | ||
| az:=dbauthz.New(nil,nil,slog.Make()) | ||
| ctrl:=gomock.NewController(s.T()) | ||
| mockStore:=dbmock.NewMockStore(ctrl) |
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading.Please reload this page.
This PR adds a
dbmetricspackage and wrapsdatabase.Storewith 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 aWrappers()method to allow us to detect what wrappers are in use.