Improve error handling in metric API #3861
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportBase: 83.14% // Head: 83.14% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3861 +/- ##
==========================================
- Coverage 83.14% 83.14% -0.01%
==========================================
Files 535 535
Lines 34145 34153 +8
==========================================
+ Hits 28391 28395 +4
- Misses 5754 5758 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
89554a7 to
3d3cf17
Compare
This comment was marked as outdated.
This comment was marked as outdated.
3d3cf17 to
9d4f445
Compare
This comment was marked as outdated.
This comment was marked as outdated.
9d4f445 to
7e3f726
Compare
This comment was marked as outdated.
This comment was marked as outdated.
7e3f726 to
b2e4eec
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
looks like it makes usage of API a bit harder (need to plan to handle explicit errors).
and it makes the code harder too.
Would rather keep exometer logic.
Current logic follows the way how lists:filter works, it does not return an error, if pattern does not match the input list elements. Trying to throw an error if lists:filter returns an empty result should be probably done on the client side instead.
src/metrics/mongoose_metrics_api.erl
Outdated
| {no_name_found, <<"No value found for queried names">>}; | ||
| Values -> | ||
| Results = [make_metric_dict_result({N, Dict}, Keys) || | ||
| {N, Dict} <- Values, filter_keys(Dict, Keys) =/= []], |
There was a problem hiding this comment.
it changes logic.
Before, we were still returning metric even if keys are not matching, which is pretty logical, could be used to list metric names and consistent between other exometer APIs.
Now you just silent remove a metric from the result set, if it has no keys.
src/metrics/mongoose_metrics_api.erl
Outdated
| {N, Dict} <- Values, filter_keys(Dict, Keys) =/= []], | ||
| case Results of | ||
| [] -> | ||
| {no_key_found, <<"No value found for queried keys">>}; |
There was a problem hiding this comment.
"No matching metrics.".
No value is usually means the metric exists but for some reason it is null.
Though an empty list of matches is already No matching metrics.
But I think the old behaviour of matching exometer API was fine.
Because how would we use API usually?
- Query for a list of metrics that match prefix, like
global. - Find the metric we are actually interested in.
- If it does not exist for some reason, we can handle it on the client side still.
Here we just adding one extra format for results (i.e. results with errors), which just making API harder to use.
Because currently it does not add any new information, useful for the user.
My opinion though.
And for keys: keys are just for filtering the results. It is an extra feature, behaviour ideally should be the same as when we don't provide keys. Also, we cannot really give a meaningless error when one key has a typo (onw for example). OK, we can, but we should define all possible keys somewhere in this case. But again, should we be that strict? Because it is check for one filter key, other filter keys still can work fine. It could cause problems when user switches between versions.
There was a problem hiding this comment.
I was asked by @jacekwegr to provide an input. In my opinion:
- We should leave the handling of the missing keys and names as it was before, as @arcusfelis suggested, because it consistently returns empty lists when metrics were not found. It would be nice (but not a must) to return an error for each missing node though, as it would be good to clearly know that a node is down.
- I played around with the API and I think that the biggest issue is the lack of the ability to query host-type metrics, e.g.
$ _build/mim1/rel/mongooseim/bin/mongooseimctl metric getMetrics --name '["global"]'works as expected, but$ _build/mim1/rel/mongooseim/bin/mongooseimctl metric getMetrics --name '["localhost"]'doesn't. Some type conversions might be necesary here. - Another note is that IMO
mongoose_metrics_apishould callmongoose_metricsrather thanexometer, let's have only one module using exometer directly for simplicity and consistency.
b2e4eec to
a874044
Compare
This comment was marked as outdated.
This comment was marked as outdated.
a874044 to
14cda31
Compare
This comment was marked as outdated.
This comment was marked as outdated.
14cda31 to
af9808e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
src/metrics/mongoose_metrics_api.erl
Outdated
| Ele | ||
| end | ||
| end, | ||
| Name). |
| get_metrics_by_name_empty_args(Config) -> | ||
| Result = get_metrics([], Config), | ||
| ParsedResult = get_ok_value([data, metric, getMetrics], Result), | ||
| lists:foreach(fun check_metric_by_type/1, ParsedResult). |
There was a problem hiding this comment.
lists:foreach is not enough here.
You should also check that the list is empty or not. Same with foreaches below.
get_metrics_by_name_empty_args(Config) ->
Result = get_metrics([], Config),
ParsedResult = get_ok_value([data, metric, getMetrics], Result),
lists:foreach(fun check_metric_by_type/1, ParsedResult),
[_|_] = ParsedResult.or
get_metrics_by_name_empty_args(Config) ->
Result = get_metrics([], Config),
ParsedResult = get_ok_value([data, metric, getMetrics], Result),
lists:foreach(fun check_metric_by_type/1, ParsedResult),
[] = ParsedResult.| ?assertEqual(length(ParsedResult2), 2). | ||
|
|
||
| get_metrics_as_dicts_empty_strings(Config) -> | ||
| %% Name is empty string |
| Result = get_cluster_metrics_as_dicts_for_nodes([<<"nonexistent">>], Config), | ||
| ParsedResult = get_ok_value([data, metric, getClusterMetricsAsDicts], Result), | ||
| [#{<<"node">> := _, <<"result">> := ResList}] = ParsedResult, | ||
| [#{<<"dict">> := [],<<"name">> := ErrorResult}] = ResList, |
| ParsedResult = get_ok_value([data, metric, getClusterMetricsAsDicts], Result), | ||
| [#{<<"node">> := _, <<"result">> := ResList}] = ParsedResult, | ||
| [#{<<"dict">> := [],<<"name">> := ErrorResult}] = ResList, | ||
| ?assert(ErrorResult == [<<"error">>,<<"nodedown">>]). |
There was a problem hiding this comment.
[<<"error">>, <<"nodedown">>]
| get_ok_value([data, metric, getClusterMetricsAsDicts], Result), | ||
| %% Key is empty string | ||
| Result2 = get_cluster_metrics_as_dicts([<<"_">>], [<<>>], [Node], Config), | ||
| get_ok_value([data, metric, getClusterMetricsAsDicts], Result2), |
There was a problem hiding this comment.
we also wanna check if the Results are empty or not
af9808e to
58bce3f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
| get_metrics_by_name_empty_string(Config) -> | ||
| Result = get_metrics([<<>>], Config), | ||
| ParsedResult = get_ok_value([data, metric, getMetrics], Result), | ||
| lists:foreach(fun check_metric_by_type/1, ParsedResult), |
There was a problem hiding this comment.
One last thing: no need for foreach, if it is an empty list.
Because it is as useful as code like that (not so useful):
lists:foreach(fun check_metric_by_type/1, []),
58bce3f to
36a6d50
Compare
|
small_tests_24 / small_tests / 36a6d50 small_tests_25 / small_tests / 36a6d50 ldap_mnesia_24 / ldap_mnesia / 36a6d50 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 36a6d50 ldap_mnesia_25 / ldap_mnesia / 36a6d50 dynamic_domains_mysql_redis_25 / mysql_redis / 36a6d50 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 36a6d50 pgsql_mnesia_24 / pgsql_mnesia / 36a6d50 pep_SUITE:pep_tests:unsubscribe_after_presence_unsubscription{error,
{{badmatch,
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_unsubscribe_after_presence_unsubscription_2616@localhost">>},
{<<"to">>,
<<"bob_unsubscribe_after_presence_unsubscription_2616@localhost/res1">>},
{<<"type">>,<<"headline">>}],
[{xmlel,<<"event">>,
[{<<"xmlns">>,
<<"http://jabber.org/protocol/pubsub#event">>}],
[{xmlel,<<"items">>,
[{<<"node">>,<<"kHrnbI93Zgb2uHHDuGkR1Q==">>}],
[{xmlel,<<"item">>,
[{<<"id">>,<<"salmon">>}],
[{xmlel,<<"entry">>,
[{<<"xmlns">>,
<<"http://www.w3.org/2005/Atom">>}],
[]}]}]}]},
{xmlel,<<"headers">>,
[{<<"xmlns">>,<<"http://jabber.org/protocol/shim">>}],
[]}]}]},
[{pep_SUITE,'-unsubscribe_after_presence_unsubscription/1-fun-0-',2,
[{file,"/home/circleci/project/big_tests/tests/pep_SUITE.erl"},
{line,384}]},
{escalus_story,story,4,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}}dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 36a6d50 internal_mnesia_25 / internal_mnesia / 36a6d50 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 36a6d50 mysql_redis_25 / mysql_redis / 36a6d50 riak_mnesia_24 / riak_mnesia / 36a6d50 pgsql_mnesia_25 / pgsql_mnesia / 36a6d50 mssql_mnesia_25 / odbc_mssql_mnesia / 36a6d50 |
This PR fixes a bug with querying host types, generates an error message for nonexistent nodes, and tests the new API behavior.