Update mixin to latest changes from grafana/postgres_exporter #1179
Conversation
There was a problem hiding this comment.
Seems correct to me, will let @v-zhuravlev take a peek as he is much fresher with our latest mixin standards.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| grafanaDashboards+:: { | |||
| 'postgres-overview.json': (import 'postgres-overview.json'), | |||
| 'postgresql-overview.json': (import 'postgresql-overview.json'), | |||
There was a problem hiding this comment.
I think this might cause some issues to existing users, best if we keep it as is if possible.
There was a problem hiding this comment.
Similar to the above, I can keep the dashboard name unchanged but consider that this is what we use in cloud-onboarding: it means that when we import the mixin from this repo, we'll need to change the dashboard name there.
|
Looks like the DCO will need to be cleaned up. What is the plan with the Grafana fork? Is it intended to be a long term fork or are there other Grafana specific reasons for it? This PR shows intent to upstream some changes so I'm just curious. We don't necessarily have mixin standards in this project so I'm open to suggestions, however I did start to refactor the mixin in #911 |
Hi Joe! I will squash commits (if @gaantunes and @v-zhuravlev are fine with that) and fix the DCO once the changes are in a good shape, if that's ok. We'd ideally avoid keeping the fork around, but we need some changes to embed it into Alloy. My intention is to start submitting some of the changes here so we can narrow the gap and at least make the fork easier to maintain. What is your plan with #911? I'm wondering if any improvement there can be applied on top of the changes we have here? |
|
I'm not set on #911 but I do want to make the mixin easier to maintain. I think that splitting queries, dashboards, alerts, and other types into separate files is helpful in that regard. |
Would you still be ok with getting this change in, then building on top of it? |
Porting a bunch of PRs accumulated over time in grafana/postgres_exporter: - grafana#11 (@v-zhuravlev) - grafana#12 (@gaantunes) - grafana#13 (@gaantunes) - grafana#14 (@gaantunes) - grafana#15 (@gaantunes) - grafana#16 (@gaantunes) - grafana#17 (@gaantunes) - grafana#20 (@gaantunes) - grafana#21 (@mshahzeb) - grafana#22 (@mshahzeb) Signed-off-by: Cristian Greco <cristian@regolo.cc>
cristiangreco
force-pushed
the
cristian/update-mixin
branch
from
8023132 to
e3c548d
Compare
|
Applied changes as per comments above, squashed commits, signed DCO, marked as ready for review. @v-zhuravlev @gaantunes would be good to have your stamp as well as original authors |
| @@ -1,5 +1,9 @@ | |||
| { | |||
| _config+:: { | |||
| postgresExporterSelector: '', | |||
| dbNameFilter: 'datname!~"template.*"', | |||
| postgresExporterSelector: 'job="integrations/postgres_exporter"', | |||
There was a problem hiding this comment.
Is this meaningful for the community? This smells like a Grafana specific selector based on the integrations prefix.
There was a problem hiding this comment.
I don't think that this should be renamed based on the dashboards/dashboards.libsonnet file.
Porting a bunch of PRs accumulated over time in grafana/postgres_exporter:
grafana#11 (@v-zhuravlev)
grafana#12 (@gaantunes)
grafana#13 (@gaantunes)
grafana#14 (@gaantunes)
grafana#15 (@gaantunes)
grafana#16 (@gaantunes)
grafana#17 (@gaantunes)
grafana#20 (@gaantunes)
grafana#21 (@mshahzeb)
grafana#22 (@mshahzeb)