refactor: move metrics out of /rules(/:id) to /rules/:id/metrics

2022-12-01 15:30:23 +01:00 · 2022-12-01 15:30:23 +01:00 · 0b324da7cb
parent 380b75d4a9
commit 0b324da7cb
7 changed files with 113 additions and 44 deletions
--- a/apps/emqx_connector/test/emqx_connector_api_SUITE.erl
+++ b/apps/emqx_connector/test/emqx_connector_api_SUITE.erl
@ -641,7 +641,7 @@ t_ingress_mqtt_bridge_with_rules(_) ->
        end
    ),
    %% and also the rule should be matched, with matched + 1:
-    {ok, 200, Rule1} = request(get, uri(["rules", RuleId]), []),
+    {ok, 200, Rule1} = request(get, uri(["rules", RuleId, "metrics"]), []),
    #{
        <<"id">> := RuleId,
        <<"metrics">> := #{
@ -748,7 +748,7 @@ t_egress_mqtt_bridge_with_rules(_) ->
    timer:sleep(100),
    wait_for_resource_ready(BridgeIDEgress, 5),
    emqx:publish(emqx_message:make(RuleTopic, Payload2)),
-    {ok, 200, Rule1} = request(get, uri(["rules", RuleId]), []),
+    {ok, 200, Rule1} = request(get, uri(["rules", RuleId, "metrics"]), []),
    #{
        <<"id">> := RuleId,
        <<"metrics">> := #{
--- a/apps/emqx_rule_engine/i18n/emqx_rule_engine_api.conf
+++ b/apps/emqx_rule_engine/i18n/emqx_rule_engine_api.conf
@ -84,6 +84,17 @@ emqx_rule_engine_api {
                          }
                  }

+    api4_1 {
+                   desc {
+                         en: "Get a rule's metrics by given Id"
+                         zh: "通过给定的 Id 获得规则的指标数据"
+                        }
+                   label: {
+                           en: "Get Metric"
+                           zh: "获得指标数据"
+                          }
+                  }
+
    api5 {
                   desc {
                         en: "Update a rule by given Id to all nodes in the cluster"
--- a/apps/emqx_rule_engine/src/emqx_rule_api_schema.erl
+++ b/apps/emqx_rule_engine/src/emqx_rule_api_schema.erl
@ -43,12 +43,6 @@ fields("rule_creation") ->
 fields("rule_info") ->
    [
        rule_id(),
-        {"metrics", sc(ref("metrics"), #{desc => ?DESC("ri_metrics")})},
-        {"node_metrics",
-            sc(
-                hoconsc:array(ref("node_metrics")),
-                #{desc => ?DESC("ri_node_metrics")}
-            )},
        {"from",
            sc(
                hoconsc:array(binary()),
@ -63,6 +57,16 @@ fields("rule_info") ->
                }
            )}
    ] ++ fields("rule_creation");
+fields("rule_metrics") ->
+    [
+        rule_id(),
+        {"metrics", sc(ref("metrics"), #{desc => ?DESC("ri_metrics")})},
+        {"node_metrics",
+            sc(
+                hoconsc:array(ref("node_metrics")),
+                #{desc => ?DESC("ri_node_metrics")}
+            )}
+    ];
 %% TODO: we can delete this API if the Dashboard not depends on it
 fields("rule_events") ->
    ETopics = emqx_rule_events:event_topics_enum(),
--- a/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl
+++ b/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl
@ -31,12 +31,18 @@
 -export([api_spec/0, paths/0, schema/1, namespace/0]).

 %% API callbacks
-export(['/rule_events'/2, '/rule_test'/2, '/rules'/2, '/rules/:id'/2, '/rules/:id/reset_metrics'/2]).
+-export([
+    '/rule_events'/2,
+    '/rule_test'/2,
+    '/rules'/2,
+    '/rules/:id'/2,
+    '/rules/:id/metrics'/2,
+    '/rules/:id/metrics/reset'/2
+]).

 %% query callback
 -export([qs2ms/2, run_fuzzy_match/2, format_rule_resp/1]).

-define(ERR_NO_RULE(ID), list_to_binary(io_lib:format("Rule ~ts Not Found", [(ID)]))).
 -define(ERR_BADARGS(REASON), begin
    R0 = err_msg(REASON),
    <<"Bad Arguments: ", R0/binary>>
@ -126,7 +132,15 @@ namespace() -> "rule".
 api_spec() ->
    emqx_dashboard_swagger:spec(?MODULE, #{check_schema => false}).

-paths() -> ["/rule_events", "/rule_test", "/rules", "/rules/:id", "/rules/:id/reset_metrics"].
+paths() ->
+    [
+        "/rule_events",
+        "/rule_test",
+        "/rules",
+        "/rules/:id",
+        "/rules/:id/metrics",
+        "/rules/:id/metrics/reset"
+    ].

 error_schema(Code, Message) when is_atom(Code) ->
    emqx_dashboard_swagger:error_codes([Code], list_to_binary(Message)).
@ -140,6 +154,9 @@ rule_test_schema() ->
 rule_info_schema() ->
    ref(emqx_rule_api_schema, "rule_info").

+rule_metrics_schema() ->
+    ref(emqx_rule_api_schema, "rule_metrics").
+
 schema("/rules") ->
    #{
        'operationId' => '/rules',
@ -230,17 +247,31 @@ schema("/rules/:id") ->
            }
        }
    };
-schema("/rules/:id/reset_metrics") ->
+schema("/rules/:id/metrics") ->
    #{
-        'operationId' => '/rules/:id/reset_metrics',
+        'operationId' => '/rules/:id/metrics',
+        get => #{
+            tags => [<<"rules">>],
+            description => ?DESC("api4_1"),
+            summary => <<"Get a Rule's Metrics">>,
+            parameters => param_path_id(),
+            responses => #{
+                404 => error_schema('NOT_FOUND', "Rule not found"),
+                200 => rule_metrics_schema()
+            }
+        }
+    };
+schema("/rules/:id/metrics/reset") ->
+    #{
+        'operationId' => '/rules/:id/metrics/reset',
        put => #{
            tags => [<<"rules">>],
            description => ?DESC("api7"),
            summary => <<"Reset a Rule Metrics">>,
            parameters => param_path_id(),
            responses => #{
-                400 => error_schema('BAD_REQUEST', "RPC Call Failed"),
-                200 => <<"Reset Success">>
+                404 => error_schema('NOT_FOUND', "Rule not found"),
+                204 => <<"Reset Success">>
            }
        }
    };
@ -363,15 +394,29 @@ param_path_id() ->
            }),
            {500, #{code => 'INTERNAL_ERROR', message => ?ERR_BADARGS(Reason)}}
    end.
-'/rules/:id/reset_metrics'(put, #{bindings := #{id := RuleId}}) ->
-    case emqx_rule_engine_proto_v1:reset_metrics(RuleId) of
-        ok ->
-            {200, <<"Reset Success">>};
-        Failed ->
-            {400, #{
-                code => 'BAD_REQUEST',
-                message => err_msg(Failed)
-            }}
+
+'/rules/:id/metrics'(get, #{bindings := #{id := Id}}) ->
+    case emqx_rule_engine:get_rule(Id) of
+        {ok, _Rule} ->
+            NodeMetrics = get_rule_metrics(Id),
+            MetricsResp =
+                #{
+                    id => Id,
+                    metrics => aggregate_metrics(NodeMetrics),
+                    node_metrics => NodeMetrics
+                },
+            {200, MetricsResp};
+        not_found ->
+            {404, #{code => 'NOT_FOUND', message => <<"Rule Id Not Found">>}}
+    end.
+
+'/rules/:id/metrics/reset'(put, #{bindings := #{id := Id}}) ->
+    case emqx_rule_engine:get_rule(Id) of
+        {ok, _Rule} ->
+            ok = emqx_rule_engine_proto_v1:reset_metrics(Id),
+            {204};
+        not_found ->
+            {404, #{code => 'NOT_FOUND', message => <<"Rule Id Not Found">>}}
    end.

 %%------------------------------------------------------------------------------
@ -394,15 +439,12 @@ format_rule_resp(#{
    enable := Enable,
    description := Descr
 }) ->
-    NodeMetrics = get_rule_metrics(Id),
    #{
        id => Id,
        name => Name,
        from => Topics,
        actions => format_action(Action),
        sql => SQL,
-        metrics => aggregate_metrics(NodeMetrics),
-        node_metrics => NodeMetrics,
        enable => Enable,
        created_at => format_datetime(CreatedAt, millisecond),
        description => Descr
--- a/apps/emqx_rule_engine/test/emqx_rule_engine_api_SUITE.erl
+++ b/apps/emqx_rule_engine/test/emqx_rule_engine_api_SUITE.erl
@ -25,7 +25,17 @@ init_per_testcase(_, Config) ->
    Config.

 end_per_testcase(_, _Config) ->
-    ok.
+    {200, #{data := Rules}} =
+        emqx_rule_engine_api:'/rules'(get, #{query_string => #{}}),
+    lists:foreach(
+        fun(#{id := Id}) ->
+            emqx_rule_engine_api:'/rules/:id'(
+                delete,
+                #{bindings => #{id => Id}}
+            )
+        end,
+        Rules
+    ).

 t_crud_rule_api(_Config) ->
    RuleID = <<"my_rule">>,
@ -49,15 +59,18 @@ t_crud_rule_api(_Config) ->
    ct:pal("RList : ~p", [Rules]),
    ?assert(length(Rules) > 0),

-    {200, Rule0} = emqx_rule_engine_api:'/rules/:id/reset_metrics'(put, #{
+    {204} = emqx_rule_engine_api:'/rules/:id/metrics/reset'(put, #{
        bindings => #{id => RuleID}
    }),
-    ?assertEqual(<<"Reset Success">>, Rule0),

    {200, Rule1} = emqx_rule_engine_api:'/rules/:id'(get, #{bindings => #{id => RuleID}}),
    ct:pal("RShow : ~p", [Rule1]),
    ?assertEqual(Rule, Rule1),

+    {200, Metrics} = emqx_rule_engine_api:'/rules/:id/metrics'(get, #{bindings => #{id => RuleID}}),
+    ct:pal("RMetrics : ~p", [Metrics]),
+    ?assertMatch(#{id := RuleID, metrics := _, node_metrics := _}, Metrics),
+
    {200, Rule2} = emqx_rule_engine_api:'/rules/:id'(put, #{
        bindings => #{id => RuleID},
        body => Params0#{<<"sql">> => <<"select * from \"t/b\"">>}
@ -68,6 +81,14 @@ t_crud_rule_api(_Config) ->
    ?assertEqual(Rule3, Rule2),
    ?assertEqual(<<"select * from \"t/b\"">>, maps:get(sql, Rule3)),

+    {404, _} = emqx_rule_engine_api:'/rules/:id'(get, #{bindings => #{id => <<"unknown_rule">>}}),
+    {404, _} = emqx_rule_engine_api:'/rules/:id/metrics'(get, #{
+        bindings => #{id => <<"unknown_rule">>}
+    }),
+    {404, _} = emqx_rule_engine_api:'/rules/:id/metrics/reset'(put, #{
+        bindings => #{id => <<"unknown_rule">>}
+    }),
+
    ?assertMatch(
        {204},
        emqx_rule_engine_api:'/rules/:id'(
@ -150,20 +171,6 @@ t_list_rule_api(_Config) ->
    QueryStr6 = #{query_string => #{<<"like_id">> => RuleID}},
    {200, Result6} = emqx_rule_engine_api:'/rules'(get, QueryStr6),
    ?assertEqual(maps:get(data, Result1), maps:get(data, Result6)),
-
-    %% clean up
-    lists:foreach(
-        fun(Id) ->
-            ?assertMatch(
-                {204},
-                emqx_rule_engine_api:'/rules/:id'(
-                    delete,
-                    #{bindings => #{id => Id}}
-                )
-            )
-        end,
-        AddIds
-    ),
    ok.

 test_rule_params() ->
--- a/changes/v5.0.12-en.md
+++ b/changes/v5.0.12-en.md
@ -14,6 +14,8 @@
 - Refactor authn API by replacing `POST /authentication/{id}/move` with `PUT /authentication/{id}/position/{position}`. [#9419](https://github.com/emqx/emqx/pull/9419).
  Same is done for `/listeners/{listener_id}/authentication/id/...`.

+- Redesign `/rules` API to make `metrics` a dedicated resources rather than being included with every response [#9461](https://github.com/emqx/emqx/pull/9461).
+
 ## Bug fixes

 - Fix that the obsolete SSL files aren't deleted after the ExHook config update [#9432](https://github.com/emqx/emqx/pull/9432).
--- a/changes/v5.0.12-zh.md
+++ b/changes/v5.0.12-zh.md
@ -8,11 +8,14 @@

 - 支持在 Apple Silicon 架构下编译苹果系统的发行版本 [#9423](https://github.com/emqx/emqx/pull/9423)。

+
 - 删除了老的共享订阅支持方式， 不再使用 `$queue` 前缀 [#9412](https://github.com/emqx/emqx/pull/9412)。
  共享订阅自 MQTT v5.0 开始已成为协议标准，可以使用 `$share` 前缀代替 `$queue`。

 - 重构认证 API，使用 `PUT /authentication/{id}/position/{position}` 代替了 `POST /authentication/{id}/move` [#9419](https://github.com/emqx/emqx/pull/9419)。

+- 重新设计了 `/rules` API，将  `metrics` 改为专用资源，而不再是包含在每个响应中 [#9461](https://github.com/emqx/emqx/pull/9461)。
+
 ## 修复

 - 修复 ExHook 更新 SSL 相关配置后，过时的 SSL 文件没有被删除的问题 [#9432](https://github.com/emqx/emqx/pull/9432)。