tests: add test for prometheus exporter #2237
Conversation
tchaikov
force-pushed
the
test-prometheus
branch
4 times, most recently
from
bf9da80 to
fa917ca
Compare
|
I'm in favor of tests of course, but wouldn't it be better to reuse: |
|
@amnonh thank you for pointing me to it. will reuse it in the next revision. |
|
v2:
@amnonh hi Amnon, could you help take another look? |
tests/unit/metrics_tester.cc
Outdated
| return make_exception_future<>(ep); | ||
| }).get(); | ||
| stop_signal.wait().get(); | ||
| { |
There was a problem hiding this comment.
the downside is that I've moved to take the configuration from a file and this part is hard-coded, it's a shame we cannot reuse Scylla's code that reads the relabel and family config from file as well.
I haven't check what would be the impact on apps/metrics_tester/test_metrics.py
There was a problem hiding this comment.
i thought about this. if we make this configurable, what do we get in terms of a better test? but if you insist, i can make it so. please let me know what you think. this part has no impact on test_metrics.py, as it only applies to the two new metrics i added to the config file.
There was a problem hiding this comment.
if your test app reads the same configuration file and compares the result (like apps/metrics_tester/test_metrics.py) it makes it easier to extend with new functionality. but it's optional
There was a problem hiding this comment.
in the latest revision, the metric_family_config:s are also configured with the yaml file. hope it's better now.
There was a problem hiding this comment.
the yaml-driven test is nice. but i think we can also encode the test cases in the code like
tests = [
TestCase(aggregate=False, expected_values=[1, 2]),
TestCase(aggregate=True, expected_values=[3])
]maybe is not that horrible in comparison with the yaml file.
| @@ -0,0 +1,234 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Besides the functionality duplication with apps/metrics_tester/test_metrics.py it looks good
There was a problem hiding this comment.
they serve different purposes. and have different structures, if we really want to unify them. i can do it, but that might need more back and forth. but i was trying to follow your suggestion of
reuse: apps/metrics_tester/metrics_tester.cc ?
or, probably we can do this in smaller steps?
There was a problem hiding this comment.
Not a must, just a code duplication
There was a problem hiding this comment.
i'd prefer do this in smaller steps. actually i have a work in progress to unify these two scripts locally. but it's not in a shape that i can upstream it.
as we fallback to creating a counter mtric, this does not change the behavior of the test. Signed-off-by: Kefu Chai <[email protected]>
the whole purpose of this tester is to serve as a prometheus exporter. so let's always start it. Signed-off-by: Kefu Chai <[email protected]>
otherwise the sharded_server would abort when the server shuts down ``` metrics_tester: /home/kefu/dev/seastar/include/seastar/core/sharded.hh:565: seastar::sharded<seastar::httpd::http_server>::~sharded() [Service = seastar::ht tpd::http_server]: Assertion `_instances.empty()' failed. Aborting. ``` Signed-off-by: Kefu Chai <[email protected]>
instead of hardwiring to {"private": "1"}, let's support "labels"
in conf.yaml, so that we can add multiple metrics with different
labels in the configuration file.
Signed-off-by: Kefu Chai <[email protected]>
before this change, only the metrics with labels of {"private": "1"}
are preserved. but if we want to test the metrics aggregation by
name, would be more convenient if we can preserve metrics with the same
name but with different labels.
so, in this change, we relax the criteria so that all metrics with
non-empty "private" labels are preserved.
Signed-off-by: Kefu Chai <[email protected]>
so that we can reuse metrics_tester for unit test -- we will add tests exercising the query parameters supported by the exporter httpd server, like `__name__` and `__aggregate__`. Signed-off-by: Kefu Chai <[email protected]>
Fixes scylladb#2233 Signed-off-by: Kefu Chai <[email protected]>
|
v3:
|
|
@amnonh hi Amnon, thank you for your review. hope it's in a better shape. could you take another look? |
|
@scylladb/seastar-maint hi maintainers, could you help merge this change? |
unlike metrics_test.cc, prometheus_test exercises the exporter server, so it tests the different query parameters supported by it.
Fixes #2233
Signed-off-by: Kefu Chai [email protected]