Improve error handling in metric API

esl · Nov 18, 2022 · 3d3cf17 · 3d3cf17
1 parent eec76fe
commit 3d3cf17
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 15 deletions.
diff --git a/big_tests/tests/graphql_metric_SUITE.erl b/big_tests/tests/graphql_metric_SUITE.erl
@@ -5,7 +5,8 @@
 -compile([export_all, nowarn_export_all]).
 
 -import(distributed_helper, [mim/0, require_rpc_nodes/1, rpc/4]).
--import(graphql_helper, [execute_command/4, get_ok_value/2, get_unauthorized/1]).
+-import(graphql_helper, [execute_command/4, get_ok_value/2, get_unauthorized/1,
+                         get_err_msg/1, get_err_code/1]).
 
 suite() ->
     MIM2NodeName = maps:get(node, distributed_helper:mim2()),
@@ -27,15 +28,20 @@ metrics_tests() ->
     [get_all_metrics,
      get_all_metrics_check_by_type,
      get_by_name_global_erlang_metrics,
+     get_nonexistent_metrics_by_name,
      get_process_queue_length,
      get_inet_stats,
      get_vm_stats_memory,
      get_all_metrics_as_dicts,
      get_by_name_metrics_as_dicts,
+     get_by_name_nonexistent_metrics_as_dicts,
      get_metrics_as_dicts_with_key_one,
+     get_metrics_with_nonexistent_key,
      get_cluster_metrics,
      get_by_name_cluster_metrics_as_dicts,
-     get_mim2_cluster_metrics].
+     get_mim2_cluster_metrics,
+     get_by_nonexisting_name_cluster_metrics_as_dicts,
+     get_cluster_metrics_for_nonexisting_nodes].
 
 domain_admin_metrics_tests() ->
     [domain_admin_get_metrics,
@@ -126,6 +132,11 @@ get_by_name_global_erlang_metrics(Config) ->
     %% Other metrics are filtered out
     undef = maps:get(ReadsKey, Map, undef).
 
+get_nonexistent_metrics_by_name(Config) ->
+    Result = get_metrics([<<"not_existing">>], Config),
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Result), <<"No value found">>)),
+    ?assertEqual(<<"no_name_found">>, get_err_code(Result)).
+
 get_process_queue_length(Config) ->
     Result = get_metrics([<<"global">>, <<"processQueueLengths">>], Config),
     ParsedResult = get_ok_value([data, metric, getMetrics], Result),
@@ -167,6 +178,11 @@ get_by_name_metrics_as_dicts(Config) ->
                           check_spiral_dict(Dict)
             end, ParsedResult).
 
+get_by_name_nonexistent_metrics_as_dicts(Config) ->
+    Result = get_metrics_as_dicts_by_name([<<"not_existing">>], Config),
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Result), <<"No value found">>)),
+    ?assertEqual(<<"no_name_found">>, get_err_code(Result)).
+
 get_metrics_as_dicts_with_key_one(Config) ->
     Result = get_metrics_as_dicts_with_keys([<<"one">>], Config),
     ParsedResult = get_ok_value([data, metric, getMetricsAsDicts], Result),
@@ -175,6 +191,11 @@ get_metrics_as_dicts_with_key_one(Config) ->
     [#{<<"key">> := <<"one">>, <<"value">> := One}] = maps:get(SentName, Map),
     true = is_integer(One).
 
+get_metrics_with_nonexistent_key(Config) ->
+    Result = get_metrics_as_dicts_with_keys([<<"not_existing">>], Config),
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Result), <<"No value found">>)),
+    ?assertEqual(<<"no_key_found">>, get_err_code(Result)).
+
 get_cluster_metrics(Config) ->
     %% We will have at least these two nodes
     Node1 = atom_to_binary(maps:get(node, distributed_helper:mim())),
@@ -205,6 +226,19 @@ get_mim2_cluster_metrics(Config) ->
     [#{<<"node">> := Node, <<"result">> := ResList}] = ParsedResult,
     check_node_result_is_valid(ResList, true).
 
+get_cluster_metrics_for_nonexisting_nodes(Config) ->
+    Result = get_cluster_metrics_as_dicts_for_nodes([<<"nonexistent">>], Config),
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Result), <<"No value found">>)),
+    ?assertEqual(<<"no_name_found">>, get_err_code(Result)).
+
+get_by_nonexisting_name_cluster_metrics_as_dicts(Config) ->
+    Result = get_cluster_metrics_as_dicts_by_name([<<"nonexistent">>], Config),
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Result), <<"No value found">>)),
+    ?assertEqual(<<"no_name_found">>, get_err_code(Result)).
+
+% get_cluster_metrics_with_nonexistent_key(Config) ->
+
+
 check_node_result_is_valid(ResList, MetricsAreGlobal) ->
     %% Check that result contains something
     Map = dict_objects_to_map(ResList),

diff --git a/src/graphql/admin/mongoose_graphql_metric_admin_query.erl b/src/graphql/admin/mongoose_graphql_metric_admin_query.erl
@@ -9,16 +9,35 @@
 
 execute(_Ctx, _Obj, <<"getMetrics">>, Args) ->
     Name = get_name(Args),
-    mongoose_metrics_api:get_metrics(Name);
+    case mongoose_metrics_api:get_metrics(Name) of
+        {no_name_found, Msg} ->
+            mongoose_graphql_helper:make_error(no_name_found, Msg, #{names => Name});
+        {ok, Info} ->
+            {ok, Info}
+    end;
 execute(_Ctx, _Obj, <<"getMetricsAsDicts">>, Args) ->
     Name = get_name(Args),
     Keys = get_keys2(Args),
-    mongoose_metrics_api:get_metrics_as_dicts(Name, Keys);
+    case mongoose_metrics_api:get_metrics_as_dicts(Name, Keys) of
+        {no_name_found, Msg} ->
+            mongoose_graphql_helper:make_error(no_name_found, Msg, #{name => Name});
+        {no_key_found, Msg} ->
+            mongoose_graphql_helper:make_error(no_key_found, Msg, #{keys => Keys});
+        {ok, Info} ->
+            {ok, Info}
+    end;
 execute(_Ctx, _Obj, <<"getClusterMetricsAsDicts">>, Args) ->
     Name = get_name(Args),
     Keys = get_keys2(Args),
     Nodes = get_nodes(Args),
-    mongoose_metrics_api:get_cluster_metrics_as_dicts(Name, Keys, Nodes).
+    case mongoose_metrics_api:get_cluster_metrics_as_dicts(Name, Keys, Nodes) of
+        {no_name_found, Msg} ->
+            mongoose_graphql_helper:make_error(no_name_found, Msg, #{name => Name});
+        {no_key_found, Msg} ->
+            mongoose_graphql_helper:make_error(no_key_found, Msg, #{keys => Keys});
+        {ok, Info} ->
+            {ok, Info}
+    end.
 
 %% get_keys is a BIF, so we have a name conflict
 get_keys2(Args) ->

diff --git a/src/metrics/mongoose_metrics_api.erl b/src/metrics/mongoose_metrics_api.erl
@@ -16,30 +16,57 @@
     {ok, #{binary() => binary() | [metric_dict_result()]}}
     | {error, binary()}.
 
--spec get_metrics(Name :: name()) -> {ok, [metric_result()]}.
+-spec get_metrics(Name :: name()) -> {ok, [metric_result()]} | {no_name_found, binary()}.
 get_metrics(Name) ->
-    Values = exometer:get_values(Name),
-    {ok, lists:map(fun make_metric_result/1, Values)}.
+    case exometer:get_values(Name) of
+        [] ->
+            {no_name_found, <<"No value found for queried name">>};
+        Values ->
+            {ok, lists:map(fun make_metric_result/1, Values)}
+    end.
 
 -spec get_metrics_as_dicts(Name :: name(), Keys :: [key()]) ->
-    {ok, [metric_dict_result()]}.
+    {ok, [metric_dict_result()]} | {no_name_found | no_key_found, binary()}.
 get_metrics_as_dicts(Name, Keys) ->
-    Values = exometer:get_values(Name),
-    {ok, [make_metric_dict_result(V, Keys) || V <- Values]}.
+    case exometer:get_values(Name) of
+        [] ->
+            {no_name_found, <<"No value found for queried names">>};
+        Values ->
+            Result1 = [make_metric_dict_result({N, Dict}, Keys) ||
+                       {N, Dict} <- Values, filter_keys(Dict, Keys) =/= []],
+            case Result1 of
+                [] ->
+                    {no_key_found, <<"No value found for queried keys">>};
+                _ ->
+                    {ok, Result1}
+            end
+    end.
 
 -spec get_cluster_metrics_as_dicts(Name :: name(), Keys :: [key()],
                                    Nodes :: [node()]) ->
-    {ok, [metric_node_dict_result()]}.
+    {ok, [metric_node_dict_result()]} | {no_name_found | no_key_found, binary()}.
 get_cluster_metrics_as_dicts(Name, Keys, Nodes) ->
     Nodes2 = existing_nodes(Nodes),
     F = fun(Node) -> rpc:call(Node, exometer, get_values, [Name]) end,
     Results = mongoose_lib:pmap(F, Nodes2),
-    {ok, [make_node_result(Node, Result, Keys)
-          || {Node, Result} <- lists:zip(Nodes2, Results)]}.
+    case lists:all(fun(Elem) -> Elem == {ok, []} end, Results) of
+        true ->
+            {no_name_found, <<"No value found for queried names or nodes">>};
+        false ->
+            Results2 = [make_node_result(Node, Result, Keys)
+                        || {Node, Result} <- lists:zip(Nodes2, Results)],
+            case lists:all(fun({ok, Elem}) -> maps:find(<<"result">>, Elem) == {ok, []} end, Results2) of
+                true ->
+                    {no_key_found, <<"No value found for queried keys">>};
+                false ->
+                    {ok, Results2}
+            end
+    end.
 
 make_node_result(Node, {ok, Values}, Keys) ->
     {ok, #{<<"node">> => Node,
-           <<"result">> => [make_metric_dict_result(V, Keys) || V <- Values]}};
+           <<"result">> => [make_metric_dict_result({N, Dict}, Keys) ||
+                            {N, Dict} <- Values, filter_keys(Dict, Keys) =/= []]}};
 make_node_result(Node, Other, _Keys) ->
     ?LOG_ERROR(#{what => metric_get_failed,
                  remote_node => Node, reason => Other}),