Add per db metrics#4183
Conversation
|
Some notes on this PR:
|
| 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.
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
benjaminjb
force-pushed
the
benjb/per-db-metrics
branch
from
bdbb541 to
954784a
Compare
Co-authored-by: Drew Sessler <36803518+dsessler7@users.noreply.github.com>
| dbs := []string{"postgres"} | ||
| if len(querySet.Databases) != 0 { | ||
| dbs = querySet.Databases | ||
| } |
There was a problem hiding this comment.
Definitely need to make a note in the documentation that if the user provides any databases for a custom query set that the default "postgres" database will not be included (unless they include it themselves).
There was a problem hiding this comment.
For sure, working on the documentation now.
| receiverName := fmt.Sprintf( | ||
| "sqlquery/%s-%s", querySet.Name, db) |
* Add per-db metrics to OTel 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 unused metrics - add kuttl test
3 participants
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]