-
Notifications
You must be signed in to change notification settings - Fork 756
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
adding codified functionality for logical replication metrics #747
adding codified functionality for logical replication metrics #747
Conversation
Signed-off-by: Zachary Caldarola <[email protected]>
a4e0f16
to
3007500
Compare
cc @SuperQ |
collector/pg_replication_slots.go
Outdated
) | ||
|
||
func init() { | ||
registerCollector("replication_slot", defaultEnabled, NewPGReplicationSlotCollector) |
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.
I just realized we were drifting from the naming conventions here. This is patterned after the node_exporter, where we make sure collectors are named based on the files. So the filename should be replication_slot.go
.
But I'm also somewhat fine with pg_<collector>.go
.
What do you think @sysadmind?
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.
I don't know that I have a strong opinion here. The tables inside the database have the pg_ prefix, so I could argue keeping the prefix. I'm not opposed to either direction.
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.
Since this is local to the package, I think we should name them after the collector to keep with the convention. pg_
seems redundant.
Signed-off-by: Zachary Caldarola <[email protected]>
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.
Overall it looks good to me. Just an edge case to cover and I'm unsure of Gauge vs Counter.
Signed-off-by: Zachary Caldarola <[email protected]>
Signed-off-by: Zachary Caldarola <[email protected]>
Signed-off-by: Zachary Caldarola <[email protected]>
Signed-off-by: Zachary Caldarola <[email protected]>
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!
Ping @sysadmind |
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.
LGTM
given the push to codify some of the queries.yaml statements, we want to add logical replication metrics. This metric will be labeled on the slot name and the metric will be the distance between the
current_wal_lsn()
and the confirmed_flush_lsn