Merge pull request #6670 from terry-xiaoyu/restore_action_metrics

fix(rule): restore metrics for actions

apps/emqx_rule_engine/src/emqx_rule_engine.app.src

@@ -1,6 +1,6 @@
 {application, emqx_rule_engine,
  [{description, "EMQ X Rule Engine"},
-  {vsn, "4.3.6"}, % strict semver, bump manually!
+  {vsn, "4.3.7"}, % strict semver, bump manually!
   {modules, []},
   {registered, [emqx_rule_engine_sup, emqx_rule_registry]},
   {applications, [kernel,stdlib,rulesql,getopt]},

apps/emqx_rule_engine/src/emqx_rule_engine.appup.src

@@ -1,52 +1,93 @@
 %% -*- mode: erlang -*-
 {VSN,
-  [{"4.3.5",[{load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}]},
-   {"4.3.0",
-    [{load_module,emqx_rule_funcs,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]},
-     {apply,{emqx_stats,cancel_update,[rule_registery_stats]}},
-     {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}]},
-   {"4.3.1",
-    [{load_module,emqx_rule_engine,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]},
-     {apply,{emqx_stats,cancel_update,[rule_registery_stats]}},
-     {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}]},
-   {"4.3.2",
-    [{load_module,emqx_rule_registry,brutal_purge,soft_purge,[]},
-     {apply,{emqx_stats,cancel_update,[rule_registery_stats]}},
-     {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}]},
-   {"4.3.3",
-    [{load_module,emqx_rule_engine,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}]},
+  [
+   {"4.3.6",
+    [ {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    ]},
+   {"4.3.5",
+    [ {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    ]},
    {"4.3.4",
-    [{load_module,emqx_rule_engine,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}]},
-   {<<".*">>,[]}],
-  [{"4.3.5",[{load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}]},
-   {"4.3.0",
-    [{load_module,emqx_rule_funcs,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]},
-     {apply,{emqx_stats,cancel_update,[rule_registery_stats]}},
-     {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}]},
-   {"4.3.1",
-    [{load_module,emqx_rule_engine,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]},
-     {apply,{emqx_stats,cancel_update,[rule_registery_stats]}},
-     {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}]},
-   {"4.3.2",
-    [{load_module,emqx_rule_registry,brutal_purge,soft_purge,[]},
-     {apply,{emqx_stats,cancel_update,[rule_registery_stats]}},
-     {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}]},
+    [ {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    ]},
    {"4.3.3",
-    [{load_module,emqx_rule_engine,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}]},
+    [ {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    ]},
+   {"4.3.2",
+    [ {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {apply,{emqx_stats,cancel_update,[rule_registery_stats]}}
+    , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    ]},
+   {"4.3.1",
+    [ {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {apply,{emqx_stats,cancel_update,[rule_registery_stats]}}
+    , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    ]},
+   {"4.3.0",
+    [ {load_module,emqx_rule_funcs,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {apply,{emqx_stats,cancel_update,[rule_registery_stats]}}
+    , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    ]},
+   {<<".*">>, []}
+  ],
+  [
+   {"4.3.6",
+    [ {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    ]},
+   {"4.3.5",
+    [ {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    ]},
    {"4.3.4",
-    [{load_module,emqx_rule_engine,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}]},
-   {<<".*">>,[]}]}.
+    [ {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    ]},
+   {"4.3.3",
+    [ {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    ]},
+   {"4.3.2",
+    [ {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {apply,{emqx_stats,cancel_update,[rule_registery_stats]}}
+    , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    ]},
+   {"4.3.1",
+    [ {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {apply,{emqx_stats,cancel_update,[rule_registery_stats]}}
+    , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    ]},
+   {"4.3.0",
+    [ {load_module,emqx_rule_funcs,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
+    , {apply,{emqx_stats,cancel_update,[rule_registery_stats]}}
+    , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    ]},
+   {<<".*">>, []}
+  ]
+}.

apps/emqx_rule_engine/src/emqx_rule_engine.erl

@@ -491,11 +491,24 @@ may_update_rule_params(Rule, Params = #{on_action_failed := OnFailed}) ->
 may_update_rule_params(Rule = #rule{actions = OldActions}, Params = #{actions := Actions}) ->
     %% prepare new actions before removing old ones
     NewActions = prepare_actions(Actions, maps:get(enabled, Params, true)),
+    ok = restore_action_metrics(OldActions, NewActions),
     _ = ?CLUSTER_CALL(clear_actions, [OldActions]),
     may_update_rule_params(Rule#rule{actions = NewActions}, maps:remove(actions, Params));
 may_update_rule_params(Rule, _Params) -> %% ignore all the unsupported params
     Rule.
 
+%% NOTE: if the user removed an action, but the action is not the last one in the list,
+%% the `restore_action_metrics/2` will not work as expected!
+restore_action_metrics([#action_instance{id = OldId} | OldActions],
+                       [#action_instance{id = NewId} | NewActions]) ->
+    emqx_rule_metrics:inc_actions_taken(NewId, emqx_rule_metrics:get_actions_taken(OldId)),
+    emqx_rule_metrics:inc_actions_success(NewId, emqx_rule_metrics:get_actions_success(OldId)),
+    emqx_rule_metrics:inc_actions_error(NewId, emqx_rule_metrics:get_actions_error(OldId)),
+    emqx_rule_metrics:inc_actions_exception(NewId, emqx_rule_metrics:get_actions_exception(OldId)),
+    restore_action_metrics(OldActions, NewActions);
+restore_action_metrics(_, _) ->
+    ok.
+
 ignore_lib_apps(Apps) ->
     LibApps = [kernel, stdlib, sasl, appmon, eldap, erts,
                syntax_tools, ssl, crypto, mnesia, os_mon,

apps/emqx_rule_engine/src/emqx_rule_runtime.erl

@@ -204,6 +204,10 @@ match_conditions({}, _Data) ->
     true.
 
 %% comparing numbers against strings
+compare(Op, undefined, undefined) ->
+    do_compare(Op, undefined, undefined);
+compare(_Op, L, R) when L == undefined; R == undefined ->
+    false;
 compare(Op, L, R) when is_number(L), is_binary(R) ->
     do_compare(Op, L, number(R));
 compare(Op, L, R) when is_binary(L), is_number(R) ->