- Notifications
You must be signed in to change notification settings - Fork1k
feat: add metrics to PGPubsub#11971
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
This stack of pull requests is managed by Graphite.Learn more about stacking. Join@spikecurtis and the rest of your teammates on |
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.
This file isn't new, it's just renamed from pubsub_test.go since it was Linux only and the metrics tests runs on any platform.
bbf6e1b to899ec80CompareThere 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, just a few questions/suggestions.
| //nolint:gosec | ||
| _,err:=p.db.ExecContext(p.ctx,`select pg_notify(`+pq.QuoteLiteral(event)+`, $1)`,message) | ||
| iferr!=nil { | ||
| p.publishesTotal.WithLabelValues("false").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.
Does false label represent error here? Is that common prom metric practice? (Just checking since I'm not that experienced adding prom 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.
The label name issuccess with valuestrue orfalse. Just trying to keep our labeling consistent within the product and that's how we do it at
| }, []string{"success"}), |
Uh oh!
There was an error while loading.Please reload this page.
| newQ.close() | ||
| p.subscribesTotal.WithLabelValues("false").Inc() | ||
| }else { | ||
| p.subscribesTotal.WithLabelValues("true").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.
How about including the event as well? Might be useful to see if there's an abnormal amount of subs/pubs/data going for a specific event.
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.
That's a bad idea because some events include the UUID of the thing, e.g. workspace, which would make the set of label values unbounded, which is a cardinal sin in metrics.
Not for this PR, but if we decide we really want that information, we could standardize event channel names to be a struct with a static root and then zero or more specific IDs, and then we could label the publishes, subscribes and messages with the static root.
Uh oh!
There was an error while loading.Please reload this page.
899ec80 toff526ccCompareMerge activity
|
Adds prometheus metrics to PGPubsub for monitoring its health and performance in production.
Related to#11950 --- additional diagnostics to help figure out what's happening