Merge pull request #6766 from zhongwencool/fix-trace-api-return

fix(trace): create_trace return new trace; delete_trace return 204

apps/emqx/src/emqx_trace/emqx_trace.erl

@@ -145,7 +145,7 @@ list(Enable) ->
     ets:match_object(?TRACE, #?TRACE{enable = Enable, _ = '_'}).
 
 -spec create([{Key :: binary(), Value :: binary()}] | #{atom() => binary()}) ->
-    ok | {error, {duplicate_condition, iodata()} | {already_existed, iodata()} | iodata()}.
+{ok, #?TRACE{}} | {error, {duplicate_condition, iodata()} | {already_existed, iodata()} | iodata()}.
 create(Trace) ->
     case mnesia:table_info(?TRACE, size) < ?MAX_SIZE of
         true ->
@@ -291,7 +291,9 @@ insert_new_trace(Trace) ->
                 #?TRACE{start_at = StartAt, type = Type, filter = Filter} = Trace,
                 Match = #?TRACE{_ = '_', start_at = StartAt, type = Type, filter = Filter},
                 case mnesia:match_object(?TRACE, Match, read) of
-                    [] -> mnesia:write(?TRACE, Trace, write);
+                    [] ->
+                        ok = mnesia:write(?TRACE, Trace, write),
+                        {ok, Trace};
                     [#?TRACE{name = Name}] -> mnesia:abort({duplicate_condition, Name})
                 end;
             [#?TRACE{name = Name}] -> mnesia:abort({already_existed, Name})

apps/emqx/test/emqx_trace_SUITE.erl

@@ -62,7 +62,7 @@ t_base_create_delete(_Config) ->
         end_at => End
     },
     AnotherTrace = Trace#{name => <<"anotherTrace">>},
-    ok = emqx_trace:create(Trace),
+    {ok, _} = emqx_trace:create(Trace),
     ?assertEqual({error, {already_existed, Name}}, emqx_trace:create(Trace)),
     ?assertEqual({error, {duplicate_condition, Name}}, emqx_trace:create(AnotherTrace)),
     [TraceRec] = emqx_trace:list(),
@@ -95,13 +95,13 @@ t_create_size_max(_Config) ->
         Name = list_to_binary("name" ++ integer_to_list(Seq)),
         Trace = [{name, Name}, {type, topic},
             {topic, list_to_binary("/x/y/" ++ integer_to_list(Seq))}],
-        ok = emqx_trace:create(Trace)
+        {ok, _} = emqx_trace:create(Trace)
               end, lists:seq(1, 30)),
     Trace31 = [{<<"name">>, <<"name31">>},
         {<<"type">>, topic}, {<<"topic">>, <<"/x/y/31">>}],
     {error, _} = emqx_trace:create(Trace31),
     ok = emqx_trace:delete(<<"name30">>),
-    ok = emqx_trace:create(Trace31),
+    {ok, _} = emqx_trace:create(Trace31),
     ?assertEqual(30, erlang:length(emqx_trace:list())),
     ok.
 
@@ -145,7 +145,7 @@ t_create_failed(_Config) ->
 
 t_create_default(_Config) ->
     {error, "name required"} = emqx_trace:create([]),
-    ok = emqx_trace:create([{<<"name">>, <<"test-name">>},
+    {ok, _} = emqx_trace:create([{<<"name">>, <<"test-name">>},
         {<<"type">>, clientid}, {<<"clientid">>, <<"good">>}]),
     [#emqx_trace{name = <<"test-name">>}] = emqx_trace:list(),
     ok = emqx_trace:clear(),
@@ -166,7 +166,7 @@ t_create_default(_Config) ->
         {<<"end_at">>, to_rfc3339(Now + 3)}
     ],
     {error, "failed by start_at >= end_at"} = emqx_trace:create(Trace2),
-    ok = emqx_trace:create([{<<"name">>, <<"test-name">>},
+    {ok, _} = emqx_trace:create([{<<"name">>, <<"test-name">>},
         {<<"type">>, topic}, {<<"topic">>, <<"/x/y/z">>}]),
     [#emqx_trace{start_at = Start, end_at = End}] = emqx_trace:list(),
     ?assertEqual(10 * 60, End - Start),
@@ -182,7 +182,7 @@ t_create_with_extra_fields(_Config) ->
         {<<"clientid">>, <<"dev001">>},
         {<<"ip_address">>, <<"127.0.0.1">>}
     ],
-    ok = emqx_trace:create(Trace),
+    {ok, _} = emqx_trace:create(Trace),
     ?assertMatch([#emqx_trace{name = <<"test-name">>, filter = <<"/x/y/z">>, type = topic}],
         emqx_trace:list()),
     ok.
@@ -191,7 +191,7 @@ t_update_enable(_Config) ->
     Name = <<"test-name">>,
     Now = erlang:system_time(second),
     End = list_to_binary(calendar:system_time_to_rfc3339(Now + 2)),
-    ok = emqx_trace:create([{<<"name">>, Name}, {<<"type">>, topic},
+    {ok, _} = emqx_trace:create([{<<"name">>, Name}, {<<"type">>, topic},
         {<<"topic">>, <<"/x/y/z">>}, {<<"end_at">>, End}]),
     [#emqx_trace{enable = Enable}] = emqx_trace:list(),
     ?assertEqual(Enable, true),
@@ -219,8 +219,8 @@ t_load_state(_Config) ->
     Finished = [{<<"name">>, <<"Finished">>}, {<<"type">>, topic},
         {<<"topic">>, <<"/x/y/3">>}, {<<"start_at">>, to_rfc3339(Now - 5)},
         {<<"end_at">>, to_rfc3339(Now)}],
-    ok = emqx_trace:create(Running),
+    {ok, _} = emqx_trace:create(Running),
-    ok = emqx_trace:create(Waiting),
+    {ok, _} = emqx_trace:create(Waiting),
     {error, "end_at time has already passed"} = emqx_trace:create(Finished),
     Traces = emqx_trace:format(emqx_trace:list()),
     ?assertEqual(2, erlang:length(Traces)),
@@ -241,7 +241,7 @@ t_client_event(_Config) ->
     Now = erlang:system_time(second),
     Start = to_rfc3339(Now),
     Name = <<"test_client_id_event">>,
-    ok = emqx_trace:create([{<<"name">>, Name},
+    {ok, _} = emqx_trace:create([{<<"name">>, Name},
         {<<"type">>, clientid}, {<<"clientid">>, ClientId}, {<<"start_at">>, Start}]),
     ok = emqx_trace_handler_SUITE:filesync(Name, clientid),
     {ok, Client} = emqtt:start_link([{clean_start, true}, {clientid, ClientId}]),
@@ -250,7 +250,7 @@ t_client_event(_Config) ->
     ok = emqtt:publish(Client, <<"/test">>, #{}, <<"1">>, [{qos, 0}]),
     ok = emqtt:publish(Client, <<"/test">>, #{}, <<"2">>, [{qos, 0}]),
     ok = emqx_trace_handler_SUITE:filesync(Name, clientid),
-    ok = emqx_trace:create([{<<"name">>, <<"test_topic">>},
+    {ok, _} = emqx_trace:create([{<<"name">>, <<"test_topic">>},
         {<<"type">>, topic}, {<<"topic">>, <<"/test">>}, {<<"start_at">>, Start}]),
     ok = emqx_trace_handler_SUITE:filesync(<<"test_topic">>, topic),
     {ok, Bin} = file:read_file(emqx_trace:log_file(Name, Now)),
@@ -279,7 +279,7 @@ t_get_log_filename(_Config) ->
         {<<"start_at">>, list_to_binary(Start)},
         {<<"end_at">>, list_to_binary(End)}
     ],
-    ok = emqx_trace:create(Trace),
+    {ok, _} = emqx_trace:create(Trace),
     ?assertEqual({error, not_found}, emqx_trace:get_trace_filename(<<"test">>)),
     ?assertEqual(ok, element(1, emqx_trace:get_trace_filename(Name))),
     ct:sleep(3000),

apps/emqx_management/src/emqx_mgmt_api_trace.erl

@@ -261,7 +261,7 @@ trace(get, _Params) ->
     end;
 trace(post, #{body := Param}) ->
     case emqx_trace:create(Param) of
-        ok -> {200};
+        {ok, Trace0} -> {200, format_trace(Trace0)};
         {error, {already_existed, Name}} ->
             {400, #{
                 code => 'ALREADY_EXISTED',
@@ -280,11 +280,27 @@ trace(post, #{body := Param}) ->
     end;
 trace(delete, _Param) ->
     ok = emqx_trace:clear(),
-    {200}.
+    {204}.
+
+format_trace(Trace0) ->
+    [
+        #{start_at := Start, end_at := End,
+            enable := Enable, type := Type, filter := Filter} = Trace1
+    ] = emqx_trace:format([Trace0]),
+    Now = erlang:system_time(second),
+    LogSize = lists:foldl(fun(Node, Acc) -> Acc#{Node => 0} end, #{},
+        mria_mnesia:running_nodes()),
+    Trace2 = maps:without([enable, filter], Trace1),
+    Trace2#{log_size => LogSize
+        , Type => iolist_to_binary(Filter)
+        , start_at => list_to_binary(calendar:system_time_to_rfc3339(Start))
+        , end_at => list_to_binary(calendar:system_time_to_rfc3339(End))
+        , status => status(Enable, Start, End, Now)
+    }.
 
 delete_trace(delete, #{bindings := #{name := Name}}) ->
     case emqx_trace:delete(Name) of
-        ok -> {200};
+        ok -> {204};
         {error, not_found} -> ?NOT_FOUND(Name)
     end.
 

apps/emqx_management/src/emqx_mgmt_cli.erl

@@ -490,7 +490,7 @@ trace_cluster_on(Name, Type, Filter, DurationS0) ->
              , end_at => list_to_binary(calendar:system_time_to_rfc3339(Now + DurationS))
              },
     case emqx_trace:create(Trace) of
-        ok ->
+        {ok, _} ->
             emqx_ctl:print("cluster_trace ~p ~s ~s successfully~n", [Type, Filter, Name]);
         {error, Error} ->
             emqx_ctl:print("[error] cluster_trace ~s ~s=~s ~p~n",

apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl

@@ -69,7 +69,7 @@ t_http_test(_Config) ->
     ],
 
     {ok, Create} = request_api(post, api_path("trace"), Header, Trace),
-    ?assertEqual(<<>>, Create),
+    ?assertMatch(#{<<"name">> := Name}, json(Create)),
 
     {ok, List} = request_api(get, api_path("trace"), Header),
     [Data] = json(List),
@@ -107,7 +107,7 @@ t_http_test(_Config) ->
 
     %% clear
     {ok, Create1} = request_api(post, api_path("trace"), Header, Trace),
-    ?assertEqual(<<>>, Create1),
+    ?assertMatch(#{<<"name">> := Name}, json(Create1)),
 
     {ok, Clear} = request_api(delete, api_path("trace"), Header),
     ?assertEqual(<<>>, Clear),
@@ -141,7 +141,7 @@ create_trace(Name, ClientId, Start) ->
     ?check_trace(
         #{timetrap => 900},
         begin
-            ok = emqx_trace:create([{<<"name">>, Name},
+            {ok, _} = emqx_trace:create([{<<"name">>, Name},
                 {<<"type">>, clientid}, {<<"clientid">>, ClientId}, {<<"start_at">>, Start}]),
             ?block_until(#{?snk_kind := update_trace_done})
         end,
@@ -206,7 +206,7 @@ do_request_api(Method, Request) ->
         {error, {shutdown, server_closed}} ->
             {error, server_closed};
         {ok, {{"HTTP/1.1", Code, _}, _Headers, Return}}
-            when Code =:= 200 orelse Code =:= 201 ->
+            when Code =:= 200 orelse Code =:= 201 orelse Code =:= 204 ->
             {ok, Return};
         {ok, {Reason, _Header, Body}} ->
             {error, Reason, Body}