-
Notifications
You must be signed in to change notification settings - Fork 450
PMM-7424 Add dbStatsCollector #329
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
PMM-7424 Add dbStatsCollector #329
Conversation
|
@percona-csalguero can you approve for running workflows on this PR? |
4aab24b to
f7bcbdf
Compare
f7bcbdf to
32a2a3e
Compare
|
Github Actions has added some linter suggestions in the code. But I've followed the coding style as used at other places in the code. It says - At many places in the current code, these scenarios already happens. For now, I'm ignoring it. Let me know if I should address these linter suggestions. |
|
Hello again. Thanks |
exporter/dbstats_collector.go
Outdated
| // Since all dbstats will have the same fields, we need to use a metric prefix (db) | ||
| // to differentiate metrics between different databases. Labels are being set only to make it easier | ||
| // to filter | ||
| prefix := db |
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.
Can we make the prefix cs + db?
I am thinking about a way to group all the metrics from this collector in the main list.
Thanks,
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'm confused about what is cs? Do you mean to append just the string like this -
prefix := "cs" + dbThere 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.
some prefix to identify the collector
dbstats_ or something like that.
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've added the dbstats_ prefix. Here is an example metric after that (testdb is name of database)-
mongodb_dbstats_testdb_indexes{cl_id="611fa5e6165e2964515910a7",cl_role="shardsvr",rs_nm="rs1",rs_state="1"} 3
I've also updated the test case and metrics mentioned in PR description.
|
@kanakagrawal thanks for making the requested changes. |
I've updated the Changelog file. I'm not sure what you meant by adding myself as a contributor? I've signed the CLA if that is what you meant. |
|
@percona-csalguero there is one issue with this PR. Even after setting the for _, metric := range makeMetrics(prefix, dbStats, d.topologyInfo.baseLabels(), d.compatibleMode) {
ch <- metric
}I see that for But they are missing for With this patch, database name is part of the metric name. So, we need to modify the |
|
OK, we are moving it to QA and checking if everything works from QA perspective |
PMM-7424
Add dbStats collector
Here are the metrics added because of this new collector (
testdbis name of database I added. Same metrics are added for all databases)When all checks have passed and code is ready for the review, bot should add
pmm-review-exportersteam as the reviewer. That would assign people from the review team automatically. Report any issues on our Forums and Discord.