-
Notifications
You must be signed in to change notification settings - Fork 6.3k
exporter: enhance unit-tests coverage for exporter #54768
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
Conversation
4b63659 to
165f04e
Compare
|
jenkins test make check |
165f04e to
52ad4a1
Compare
e6b722c to
594d012
Compare
| bool sort; | ||
| sort = sort_metrics ? true : g_conf().get_val<bool>("exporter_sort_metrics"); |
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.
There is a lot of ternary operations for these things. Why not just extract get_val from this function and move it to request_loop so we don't have this many ternaries.
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 done this to make it easier for unit tests as well as they call this function for exporting metrics.
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 understand what you did there. I'm just saying that since we are using parameters for dump_asok_metrics, might be worth to move generation of those parameters out of it so that the logic is not too hacky.
src/test/exporter/test_exporter.cc
Outdated
| } | ||
|
|
||
| TEST(Exporter, add_fixed_name_metrics) { | ||
| std::string metric_name = "ceph_data_sync_from_zone2-zg1-realm1_fetch_bytes"; |
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 have more test cases?
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.
Will add a couple more
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 a few more for metrics name used in RGW grafana dashboards
594d012 to
8405000
Compare
|
jenkins test make check |
src/test/exporter/test_exporter.cc
Outdated
| std::string metric_name = "ceph_data_sync_from_zone2-zg1-realm1_fetch_bytes"; | ||
|
|
||
| DaemonMetricCollector &collector = collector_instance(); | ||
| std::pair<labels_t, std::string> new_metric = collector.add_fixed_name_metrics(metric_name); | ||
| labels_t expected_labels = {{"source_zone", "\"zone2-zg1-realm1\""}}; | ||
| std::string expected_metric_name = "ceph_data_sync_from_zone_fetch_bytes"; | ||
| EXPECT_EQ(new_metric.first, expected_labels); | ||
| ASSERT_TRUE(new_metric.second == expected_metric_name); | ||
|
|
||
| // Test 2 | ||
| metric_name = "ceph_data_sync_from_zone2-zg1-realm1_fetch_bytes_sum"; | ||
| new_metric = collector.add_fixed_name_metrics(metric_name); | ||
| expected_labels = {{"source_zone", "\"zone2-zg1-realm1\""}}; | ||
| expected_metric_name = "ceph_data_sync_from_zone_fetch_bytes_sum"; | ||
| EXPECT_EQ(new_metric.first, expected_labels); | ||
| ASSERT_TRUE(new_metric.second == expected_metric_name); | ||
|
|
||
| // Test 3 | ||
| metric_name = "ceph_data_sync_from_zone2-zg1-realm1_poll_latency_sum"; | ||
| new_metric = collector.add_fixed_name_metrics(metric_name); | ||
| expected_labels = {{"source_zone", "\"zone2-zg1-realm1\""}}; | ||
| expected_metric_name = "ceph_data_sync_from_zone_poll_latency_sum"; | ||
| EXPECT_EQ(new_metric.first, expected_labels); | ||
| ASSERT_TRUE(new_metric.second == expected_metric_name); |
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.
| std::string metric_name = "ceph_data_sync_from_zone2-zg1-realm1_fetch_bytes"; | |
| DaemonMetricCollector &collector = collector_instance(); | |
| std::pair<labels_t, std::string> new_metric = collector.add_fixed_name_metrics(metric_name); | |
| labels_t expected_labels = {{"source_zone", "\"zone2-zg1-realm1\""}}; | |
| std::string expected_metric_name = "ceph_data_sync_from_zone_fetch_bytes"; | |
| EXPECT_EQ(new_metric.first, expected_labels); | |
| ASSERT_TRUE(new_metric.second == expected_metric_name); | |
| // Test 2 | |
| metric_name = "ceph_data_sync_from_zone2-zg1-realm1_fetch_bytes_sum"; | |
| new_metric = collector.add_fixed_name_metrics(metric_name); | |
| expected_labels = {{"source_zone", "\"zone2-zg1-realm1\""}}; | |
| expected_metric_name = "ceph_data_sync_from_zone_fetch_bytes_sum"; | |
| EXPECT_EQ(new_metric.first, expected_labels); | |
| ASSERT_TRUE(new_metric.second == expected_metric_name); | |
| // Test 3 | |
| metric_name = "ceph_data_sync_from_zone2-zg1-realm1_poll_latency_sum"; | |
| new_metric = collector.add_fixed_name_metrics(metric_name); | |
| expected_labels = {{"source_zone", "\"zone2-zg1-realm1\""}}; | |
| expected_metric_name = "ceph_data_sync_from_zone_poll_latency_sum"; | |
| EXPECT_EQ(new_metric.first, expected_labels); | |
| ASSERT_TRUE(new_metric.second == expected_metric_name); | |
| std::vector<string> metrics = { | |
| "ceph_data_sync_from_zone2-zg1-realm1_fetch_bytes", | |
| "ceph_data_sync_from_zone2-zg1-realm1_fetch_bytes_sum", | |
| "ceph_data_sync_from_zone2-zg1-realm1_poll_latency_sum", | |
| } | |
| std::string metric_name = "ceph_data_sync_from_zone2-zg1-realm1_fetch_bytes"; | |
| for (string &metric_name : metrics) { | |
| DaemonMetricCollector &collector = collector_instance(); | |
| std::pair<labels_t, std::string> new_metric = collector.add_fixed_name_metrics(metric_name); | |
| labels_t expected_labels = {{"source_zone", "\"zone2-zg1-realm1\""}}; | |
| std::string expected_metric_name = "ceph_data_sync_from_zone_fetch_bytes"; | |
| EXPECT_EQ(new_metric.first, expected_labels); | |
| ASSERT_TRUE(new_metric.second == expected_metric_name); | |
| } |
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.
Will update thanks!
| std::string dump_response; | ||
| std::string schema_response; | ||
| dump_asok_metrics(false, -1, true, dump_response, schema_response, true); |
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.
This is pretty hacky and where it would be worth to move dump_response and schema_response generation.
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.
Where you suggest to move it to?
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.
dump_asok pseudo code looks something like:
for (daemon in daemons) {
ping
get_dump_response
get_schema_response
get_config_show
// do stuff
}Then for each daemon iteration you have 4 variables that can be exchanged for that. Even with that in a testing phase all daemons will have same inputs which is not ok but whatever, works for this tests now.
I propose to change dump_asok_metrics a extrat the loop so that you have
for (daemon in daemons) {
ping
get_dump_response
get_schema_response
get_config_show
parse_daemon_metrics(&dump, &schema, &config);
}so that in the test you can call parse_daemon_metrics that is what you care about, pinging is not a input this way, you remove unnecessary branches checking if this method is running in "test" mode or "normal" mode.
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.
Is good your idea @pereman2 - Even i think that probably "parse_daemon_metrics" must be a class for example, "generic_metrics_parser".
As we have "special" parsing needs depending of the daemon, probably to have inherited classes for these daemons, for example "rgw_metrics_parser" could also simplify and make more consistent the code.
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.
8405000 to
a444e36
Compare
|
jenkins test make check |
jmolmo
left a comment
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.
Please test and move the test for "add_fixed_name_metrics" to the PR where the modifications in the function are set.
I think that for the rest , probably we will need to agree a refactor in code and tests.
| ASSERT_TRUE(collector.metrics.find(expectedMetrics) != std::string::npos); | ||
| } | ||
|
|
||
| TEST(Exporter, add_fixed_name_metrics) { |
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.
add_fixed_named_metrics is private
src/test/exporter/test_exporter.cc
Outdated
| } | ||
|
|
||
| TEST(Exporter, add_fixed_name_metrics) { | ||
| std::vector<string> metrics = { |
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.
using a tuple can simplify the test:
TEST(Exporter, add_fixed_name_metrics) {
// test metrics are {metric_name_coming_from_rgw, rgw_zone, metric_name_output}
std::vector<std::tuple<std::string, std::string, std::string>> metrics {
{"ceph_data_sync_from_zone2-zg1-realm1_fetch_bytes", "zone2-zg1-realm1", "ceph_data_sync_from_zone_fetch_bytes"},
{"ceph_data_sync_from_zone2-zg1-realm1_fetch_bytes_sum", "zone2-zg1-realm1", "ceph_data_sync_from_zone_fetch_bytes_sum"},
{"ceph_data_sync_from_zone2-zg1-realm1_poll_latency_sum", "zone2-zg1-realm1", "ceph_data_sync_from_zone_poll_latency_sum"},
{"ceph_data_sync_from_zone2_fetch_bytes_count","zone2", "ceph_data_sync_from_zone_fetch_bytes"},
{"ceph_data_sync_from_zone2_with_underscore_fetch_bytes_count","zone2_with_underscore", "ceph_data_sync_from_zone_fetch_bytes_count"},
};
DaemonMetricCollector &collector = collector_instance();
for (auto& test_metric : metrics) {
std::pair<labels_t, std::string> new_metric = collector.add_fixed_name_metrics(std::get<0>(test_metric));
labels_t expected_labels = {{"source_zone", std::get<1>(test_metric)}};
std::string expected_metric_name = std::get<2>(test_metric);
EXPECT_EQ(new_metric.first, expected_labels);
ASSERT_TRUE(new_metric.second == expected_metric_name);
}
}
I've already established a setup for unit tests here, and there are additional modifications needed for the DaemonMetricCollector. I would prefer to incorporate any necessary unit tests into this existing PR |
a444e36 to
cfd0c18
Compare
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.
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/exporter/test_exporter.cc:1423:17: error: unknown type name 'string'; did you mean 'boost_swap_impl::string'?
std::vector<string> metrics = {
^~~~~~
boost_swap_impl::string
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stringfwd.h:79:33: note: 'boost_swap_impl::string' declared here
typedef basic_string<char> string;
^
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/exporter/test_exporter.cc:1427:6: error: expected ';' at end of declaration
}
^
;
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/exporter/test_exporter.cc:1429:10: error: use of undeclared identifier 'string'; did you mean 'stdin'?
for (string &metric_name : metrics) {
^~~~~~
stdin
did you run tests locally?
cfd0c18 to
16af936
Compare
Yes, I rebased & just fixed that issue & added the unit tests for the regex changes introduced here #56421 |
16af936 to
e9e7231
Compare
a73517e to
1f61398
Compare
pereman2
left a comment
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.
did you run tests locally?
Yes, I rebased & just fixed that issue & added the unit tests for the regex changes introduced here #56421
160/293 Test #214: unittest_exporter .........................***Failed 0.63 sec
did not load config file, using default settings.
[==========] Running 4 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 4 tests from Exporter
[ RUN ] Exporter.promethize
[ OK ] Exporter.promethize (4 ms)
[ RUN ] Exporter.check_labels_and_metric_name
[ OK ] Exporter.check_labels_and_metric_name (0 ms)
[ RUN ] Exporter.dump_asok_metrics
2024-04-12T11:17:01.527+0000 7fc469d1dac0 -1 Errors while parsing config file!
2024-04-12T11:17:01.527+0000 7fc469d1dac0 -1 can't open ceph.conf: (2) No such file or directory
2024-04-12T11:17:01.527+0000 7fc469d1dac0 -1 Errors while parsing config file!
2024-04-12T11:17:01.527+0000 7fc469d1dac0 -1 can't open ceph.conf: (2) No such file or directory
[ OK ] Exporter.dump_asok_metrics (64 ms)
[ RUN ] Exporter.add_fixed_name_metrics
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/exporter/test_exporter.cc:1437: Failure
Expected equality of these values:
new_metric.first
Which is: { ("source_zone", "\"data_sync_from_zone2-zg1-realm1_fetch_bytes\"") }
expected_labels
Which is: { ("source_zone", "\"zone2-zg1-realm1\"") }
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/exporter/test_exporter.cc:1438: Failure
Value of: new_metric.second == expected_metric_name
Actual: false
Expected: true
[ FAILED ] Exporter.add_fixed_name_metrics (0 ms)
[----------] 4 tests from Exporter (68 ms total)
[----------] Global test environment tear-down
[==========] 4 tests from 1 test suite ran. (68 ms total)
[ PASSED ] 3 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] Exporter.add_fixed_name_metrics
1 FAILED TEST
Yeah regex is almost impossible to construct without any restriction for zone name. also it seems like prometheus module was following a pattern or atleast assuming it so I'm gonna follow the same in exporter now & update |
I have been 2 weeks saying you to execute your test by yourself and pointing you that the regex does not work.Please :
You would have my complete opposition to merge this unless you do what I am saying. I am not going to review nothing more until i see the code, the test,and the execution of the test in pr 56421 |
I wanted to touch base regarding the discussion on zone name restrictions. I've reached out to the RGW team to clarify the pattern for zone names. In the meantime, I'm considering using the regex pattern that was previously employed in the Prometheus module, as it proved effective. Regarding the PR migration, I understand your point. I think it would be preferable to incorporate the changes from PR #56421 into this PR and close the former. Since this PR focuses mainly on introducing unit tests, while the other PR contains only the regex fix, consolidating them here would streamline the process. How does that sound to you? @jmolmo |
|
@avanthakkar please refer to: |
Yes I understood the point, but I'm saying if we could have this trivial change of regex in this PR itself it'd be perfect, bcz this PR has unit test setup also & it already has regex changes , maybe some modification would be required but mostly its good. The PR #56421 we can close in favor of this PR including the changes required, instead of moving 100s of lines of code changes to other PR we can simply move a couple lines changes here itself. |
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
1f61398 to
fe693d0
Compare
pereman2
left a comment
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.
[ RUN ] Exporter.dump_asok_metrics
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/exporter/test_exporter.cc:1293: Failure
Value of: collector.metrics.find(expectedMetrics) != std::string::npos
Actual: false
Expected: true
Fixes: https://tracker.ceph.com/issues/63849 Signed-off-by: avanthakkar <avanjohn@gmail.com>
fe693d0 to
ccdd877
Compare
Yes, I was fixing it probably should've marked PR WIP. Anyways I've fixed the tests now & they all passed locally |
|
jenkins test windows |
|
jenkins test make check arm64 |
Fixes: https://tracker.ceph.com/issues/63849
Signed-off-by: avanthakkar avanjohn@gmail.com
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windowsjenkins test rook e2e