- Notifications
You must be signed in to change notification settings - Fork926
feat: add prometheus metric for tracking user statuses#15281
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
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.
LGTM, a few nits.
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.
Uh oh!
There was an error while loading.Please reload this page.
3de98c2 intomainUh oh!
There was an error while loading.Please reload this page.
| }, testutil.WaitShort, testutil.IntervalSlow) | ||
| } | ||
| require.Eventually(t, checkFn, testutil.WaitShort, testutil.IntervalFast) |
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.
Hhmm, do we needrequire.Eventually with quartz now implemented (thank you for doing this!)?
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 was my assumption thatAdvanceNext().MustWait() only waits for the ticker to fire, then the actual gathering code runs async. I guess I could technically make it block until the end usingAfterFunc, but that felt kinda janky. My assumption could definitely be wrong tho, in my testing the metrics seemed to always be ready on the first try but I didn't want to risk it.
| ctx, cancelFunc := context.WithCancel(ctx) | ||
| done := make(chan struct{}) | ||
| ticker := clk.NewTicker(duration) | ||
| go func() { | ||
| defer close(done) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| } | ||
| gauge.Reset() | ||
| //nolint:gocritic // This is a system service that needs full access | ||
| //to the users table. | ||
| users, err := db.GetUsers(dbauthz.AsSystemRestricted(ctx), database.GetUsersParams{}) | ||
| if err != nil { | ||
| logger.Error(ctx, "get all users for prometheus metrics", slog.Error(err)) | ||
| continue | ||
| } | ||
| for _, user := range users { | ||
| gauge.WithLabelValues(string(user.Status)).Inc() | ||
| } | ||
| } |
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.
Instead of doing this you can just useclock.TickerFunc, it removes any timing problems in the tests as well
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, that makes more sense
Adds the ability to track total users in the platform by status. This can help for seeing how license seat usage is being used over time.
Closes#15278