-
Notifications
You must be signed in to change notification settings - Fork 15
[dbsql] [ffapi] Metrics for Go and DB Stats #205
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: hfuss <[email protected]>
Signed-off-by: hfuss <[email protected]>
Signed-off-by: hfuss <[email protected]>
EnriqueL8
left a comment
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.
Thanks @onelapahead - this is great!
A few comments
| opts.dbName = "default" | ||
| } | ||
|
|
||
| metricsRegistry.MustRegisterCollector(collectors.NewDBStatsCollector(db, opts.dbName)) |
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.
Info on stats https://pkg.go.dev/database/sql#DBStats
| as.MetricsRegistry.MustRegisterCollector(collectors.NewProcessCollector(collectors.ProcessCollectorOpts{})) | ||
| as.MetricsRegistry.MustRegisterCollector(collectors.NewGoCollector()) | ||
| } | ||
| as.MetricsRegistry.MustRegisterCollector(collectors.NewBuildInfoCollector()) |
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.
Important comment on this collector:
The labels will only have meaningful values if the binary is built with Go module support and from source code retrieved from the source repository (rather than the local file system).
| alwaysPaginate bool | ||
| handleYAML bool | ||
| monitoringEnabled bool | ||
| goProcessMetricsEnabled bool |
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 is whole pkg is already Golang
| goProcessMetricsEnabled bool | |
| processMetricsEnabled bool |
| if opts.dbName == "" { | ||
| opts.dbName = "default" | ||
| } |
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.
Confused on this one, it feels like it should be mandatory to supply a dbName
Go metrics can be useful when needing to understand heap usage and how much GC is consuming resources.
DB metrics are self-explanatory.
The ability to enable the default Go/process metrics on the API server, and for clients to manually register/enable DB metrics.