Skip to content
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

Merged
merged 6 commits into from Feb 7, 2023

Conversation

sheldor14
Copy link
Contributor

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

Signed-off-by: Zachary Caldarola <zachary.caldarola@reddit.com>
@sheldor14
Copy link
Contributor Author

cc @SuperQ

collector/pg_replication_slots.go Outdated Show resolved Hide resolved
)

func init() {
registerCollector("replication_slot", defaultEnabled, NewPGReplicationSlotCollector)
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

collector/pg_replication_slots.go Outdated Show resolved Hide resolved
collector/pg_replication_slots.go Outdated Show resolved Hide resolved
@SuperQ SuperQ requested a review from sysadmind January 25, 2023 08:40
Signed-off-by: Zachary Caldarola <zachary.caldarola@reddit.com>
@sheldor14 sheldor14 requested review from SuperQ and removed request for sysadmind January 25, 2023 16:15
Copy link
Contributor

@sysadmind sysadmind left a 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.

collector/pg_replication_slots.go Outdated Show resolved Hide resolved
collector/pg_replication_slots.go Outdated Show resolved Hide resolved
Signed-off-by: Zachary Caldarola <zmc2005@gmail.com>
Signed-off-by: Zachary Caldarola <zmc2005@gmail.com>
Signed-off-by: Zachary Caldarola <zmc2005@gmail.com>
Signed-off-by: Zachary Caldarola <zmc2005@gmail.com>
@sheldor14 sheldor14 requested review from sysadmind and removed request for SuperQ January 31, 2023 03:06
Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@SuperQ
Copy link
Contributor

SuperQ commented Feb 2, 2023

Ping @sysadmind

Copy link
Contributor

@sysadmind sysadmind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants