Add per db metrics #4183
Add per db metrics #4183
Conversation
|
Some notes on this PR:
|
| @@ -2165,6 +2170,12 @@ spec: | |||
| type: string | |||
| type: array | |||
| type: object | |||
| perDBMetricTargets: | |||
There was a problem hiding this comment.
🤔 IIUC, this and customQueries only make sense at/on Postgres. Is there a way to arrange structs so the PGAdmin API doesn't have these fields? Not a blocker; v1beta1.
There was a problem hiding this comment.
I didn't want to make any too big changes with the spec, but I wonder if the pgadmin and postgrescluster versions will diverge even more (or if we'll eventually want to slice up instrumentation by target-type, eg., postgres-instrumentation, pgbouncer-instrumentation, pgadmin-instrumentation).
There was a problem hiding this comment.
Hmm yeah good point... Probably makes sense to at least have a pgadmin-instrumentation and a postgrescluster-instrumentation and then pick and choose which structs make sense in each
There was a problem hiding this comment.
Should I add "taking apart the instrumentation struct" to this PR?
There was a problem hiding this comment.
Might make sense if you have the bandwidth...
There was a problem hiding this comment.
Given the other work due this sprint, I'm going to make a ticket for this topic and not include it in this PR.
There was a problem hiding this comment.
Made that other ticket.
|
@dsessler7 This failed at finding patroni logs -- didn't we run into that before? |
It looks like it is still using the 5.8.1 postgres-operator image even though the 5.8.2 image is specified for UPDATE: Okay, it looks like your branch still has the 5.8.1 image set in the GH action, so I think you just need to rebase on main. |
| "datasource": fmt.Sprintf( | ||
| `host=localhost dbname=%s port=5432 user=%s password=${env:PGPASSWORD}`, | ||
| db, | ||
| MonitoringUser), |
There was a problem hiding this comment.
❓ Will the monitoring user always have access to whatever databases the user specifies?
There was a problem hiding this comment.
Looking into this -- I got the metrics I expected, but why?
There was a problem hiding this comment.
It might be because we GRANT pg_monitor to ccp_monitoring; in the setup.
| // Default behavior is to target `postgres`. | ||
| // --- | ||
| // +optional | ||
| Database string `json:"database,omitempty"` |
There was a problem hiding this comment.
Do we want the user to be able to specify multiple databases for a given batch of custom queries? 🤔
There was a problem hiding this comment.
Good question, I went back and forth a bit, as a user could just add another customQueries section with a ref to the same configMap/queries and a new db -- but I'm not sold on that interface as more correct than just adding an array of DBs to this field.
There was a problem hiding this comment.
Want to table this discussion and get feedback from the demo tomorrow?
This PR adds changes to allow per-db metrics in OTel: - change API for per-db metrics - add default metrics for per-db metrics based on pgmonitor 5.2.1 - remove unusused metrics - add kuttl test
bdbb541 to
954784a
Compare
Co-authored-by: Drew Sessler <36803518+dsessler7@users.noreply.github.com>
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
No per-db metrics
What is the new behavior (if this is a feature change)?
Breaking change (fix or feature that would cause existing functionality to change)
Add per-db metrics (based on pgMonitor v5.2.1)
Allow per-db custom metrics
Other Information:
Issues: [PGO-21]