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

replSetGetConfigCollector #295

Merged
merged 29 commits into from
Nov 11, 2024
Merged

Conversation

hiroshi
Copy link
Contributor

@hiroshi hiroshi commented Jun 12, 2021

Add replSetGetConfigCollector.
This is meant to resolve #292

It export metrics like this:

mongodb_cfg_members_priority{cl_id="000000000000000000000000",cl_role="shardsvr",member_idx="db-1:27017",rs_nm="MyRep",rs_state="2"} 3
mongodb_cfg_members_hidden{cl_id="000000000000000000000000",cl_role="shardsvr",member_idx="db-1:27017",rs_nm="MyRep",rs_state="2"} 0

Usage example

Count number of members eligible for primary
count(mongodb_cfg_members_priority > 0 and mongodb_cfg_members_hidden == 0 and ignoring(member_state) mongodb_members_health == 1)
(I use this for alerting when the number is below 2)

TODO

  • Fix lint errors
  • Add replset_config_collector_test.go

  • Tests passed.
  • Fix conflicts with target branch.
  • Update jira ticket description if needed.
  • Attach screenshots/console output to confirm new behavior to jira ticket, if applicable.

When all checks have passed and code is ready for the review, bot should add pmm-review-exporters team as the reviewer. That would assign people from the review team automatically. Report any issues on our Forums and Discord.

@denisok denisok requested review from a team, percona-csalguero and JiriCtvrtka and removed request for a team July 16, 2021 15:43
Copy link
Contributor

@percona-csalguero percona-csalguero left a comment

Choose a reason for hiding this comment

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

Hi,
Thanks for the fix.
Could you address the linter suggestions to set a base for further improvements?

Thanks !

@percona-csalguero
Copy link
Contributor

Hi. Just wanted to let you know I haven't forget about this.
I've just been busy and I would like to manually test it before merging it.
Is it ready for review? It appears as draft.

Regards

@hiroshi
Copy link
Contributor Author

hiroshi commented Aug 2, 2021

Hi, thanks for your attention.

I think at least a test is needed to be reviewable. So it remains as a draft.

I'll add a test when I have a time. Or, if you think this changes are usable I appreciate you take over this.

@percona-csalguero
Copy link
Contributor

Hello again.
I've checked out your branch and it looks good. I need to ask you for some small changes:

  • Address the linter suggestions
  • Add some tests, using this collector against the different server types we have in the docker-compose file.

Thanks for your help.

@JiriCtvrtka
Copy link
Contributor

@hiroshi Do you planning write some tests here? I think everything else is fine and can be merged then.

@hiroshi
Copy link
Contributor Author

hiroshi commented Sep 19, 2021

EDIT: I gave up running linter locally. Tried to fix the linter error and checked it by pushing commits.. So you can ignore this comment.


#295 (review)

Could you address the linter suggestions to set a base for further improvements?

I was trying to run linter on my local Apple Silicon mac, but not succeeded yet.
Image from Gyazo

I think this issue is related #286

How do I run those linter locally to check if my change is OK?

@hiroshi
Copy link
Contributor Author

hiroshi commented Sep 19, 2021

The line where the linter reported an error,
https://github.com/percona/mongodb_exporter/pull/295/files#diff-36945caa3d7bc040bc4a27f3cd597e18399e05ec94a87f344ac74eac5efeb5bcR53

if e, ok := err.(mongo.CommandError); ok {

is a almost identiacal to the line,
if e, ok := err.(mongo.CommandError); ok {

but it seems no linter error here.

I'm not sure what difference cause the linter error...

@hiroshi
Copy link
Contributor Author

hiroshi commented Sep 19, 2021

Also I was trying to run test on my local Apple silicon mac, but unable to run them.

Image from Gyazo

I created another pull request to address it -> #342

@hiroshi
Copy link
Contributor Author

hiroshi commented Sep 19, 2021

I fear that to run lint and test on my Apple silicon mac is unneccesary harder than fix lint and add test itself...
I hope someone taking over this pull request, fix lint and add test for it.
Or should I try on a linux environment, utilize docker for lint and test or fix all problem on Apple silicon mac?
Sorry, I'v just been frustrated...

@hiroshi
Copy link
Contributor Author

hiroshi commented Sep 20, 2021

#295 (review)

Could you address the linter suggestions to set a base for further improvements?

@percona-csalguero I fixed the linter error in 54ded8c.

EDIT: No, this causes segfault on d.logger.Errorf("cannot get replSetGetConfig: %s", err)
https://github.com/percona/mongodb_exporter/pull/295/checks

var e *mongo.CommandError
if errors.As(err, &e) {
if e.Code == replicationNotYetInitialized || e.Code == replicationNotEnabled {
return
}
}
d.logger.Errorf("cannot get replSetGetConfig: %s", err)

exporter/exporter.go Outdated Show resolved Hide resolved
exporter/exporter.go Outdated Show resolved Hide resolved
@BupycHuk BupycHuk removed the request for review from percona-csalguero November 11, 2024 07:27
@BupycHuk BupycHuk marked this pull request as ready for review November 11, 2024 07:40
@BupycHuk BupycHuk requested a review from a team as a code owner November 11, 2024 07:40
@BupycHuk BupycHuk requested review from ademidoff and idoqo and removed request for a team November 11, 2024 07:40
Comment on lines 28 to 31
// const (
// replicationNotEnabled = 76
// replicationNotYetInitialized = 94
// )
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we keeping this for purpose?

exporter/replset_config_collector.go Outdated Show resolved Hide resolved
exporter/replset_config_collector.go Show resolved Hide resolved
@BupycHuk BupycHuk enabled auto-merge (squash) November 11, 2024 10:59
@BupycHuk BupycHuk merged commit 0239f77 into percona:main Nov 11, 2024
13 checks passed
@hiroshi
Copy link
Contributor Author

hiroshi commented Nov 11, 2024

Thanks for all.

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

Successfully merging this pull request may close these issues.

How to monitor number of members eligible for primary?
6 participants