Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

C2s/metrics #3800

Merged
merged 2 commits into from
Oct 11, 2022
Merged

C2s/metrics #3800

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8e3477
Fix metrics suite
Oct 7, 2022
a5c97ac
Optimize send_element/3
Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions big_tests/default.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,10 @@
{suites, "tests", mam_SUITE}.
{suites, "tests", mam_proper_SUITE}.
{suites, "tests", mam_send_message_SUITE}.
% {suites, "tests", metrics_api_SUITE}.
% {suites, "tests", metrics_c2s_SUITE}.
{suites, "tests", metrics_api_SUITE}.
{suites, "tests", metrics_c2s_SUITE}.
{suites, "tests", metrics_register_SUITE}.
% {suites, "tests", metrics_roster_SUITE}.
{suites, "tests", metrics_roster_SUITE}.
{suites, "tests", metrics_session_SUITE}.
{suites, "tests", mod_blocking_SUITE}.
{suites, "tests", mod_event_pusher_http_SUITE}.
Expand Down
8 changes: 4 additions & 4 deletions big_tests/dynamic_domains.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,15 @@
{suites, "tests", mam_proper_SUITE}.
{suites, "tests", mam_send_message_SUITE}.

% {suites, "tests", metrics_c2s_SUITE}.
{suites, "tests", metrics_c2s_SUITE}.

{suites, "tests", metrics_register_SUITE}.

% {suites, "tests", metrics_roster_SUITE}.
{suites, "tests", metrics_roster_SUITE}.

% {suites, "tests", metrics_session_SUITE}.
{suites, "tests", metrics_session_SUITE}.

% {suites, "tests", metrics_api_SUITE}.
{suites, "tests", metrics_api_SUITE}.

{suites, "tests", mod_blocking_SUITE}.

Expand Down
42 changes: 23 additions & 19 deletions big_tests/tests/metrics_api_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,14 @@ one_client_just_logs_in(Config) ->
(Config, metrics_helper:userspec(1, Config),
fun(_User1) -> end_of_story end,
%% A list of metrics and their expected relative increase
[{xmppIqSent, 0},
{xmppIqReceived, 0},
[{xmppIqSent, 0 + user_alpha(2)},
{xmppIqReceived, 0 + user_alpha(2)},
{xmppMessageSent, 0},
{xmppMessageReceived, 0},
{xmppPresenceSent, 0 + user_alpha(1)},
{xmppPresenceReceived, 0 + user_alpha(1)},
{xmppStanzaSent, 0 + user_alpha(1)},
{xmppStanzaReceived, 0 + user_alpha(1)},
{xmppStanzaSent, 0 + user_alpha(3)},
{xmppStanzaReceived, 0 + user_alpha(3)},
{sessionSuccessfulLogins, 0 + user_alpha(1)},
{sessionLogouts, 0 + user_alpha(1)}
]).
Expand All @@ -126,12 +126,14 @@ two_clients_just_log_in(Config) ->
instrumented_story
(Config, metrics_helper:userspec(1, 1, Config),
fun(_User1, _User2) -> end_of_story end,
[{xmppMessageSent, 0},
[{xmppIqSent, 0 + user_alpha(4)},
{xmppIqReceived, 0 + user_alpha(4)},
{xmppMessageSent, 0},
{xmppMessageReceived, 0},
{xmppStanzaSent, 0 + user_alpha(2)},
{xmppStanzaReceived, 0 + user_alpha(2)},
{xmppPresenceSent, 0 + user_alpha(2)},
{xmppPresenceReceived, 0 + user_alpha(2)},
{xmppStanzaSent, 0 + user_alpha(6)},
{xmppStanzaReceived, 0 + user_alpha(6)},
{sessionSuccessfulLogins, 0 + user_alpha(2)},
{sessionLogouts, 0 + user_alpha(2)}
]).
Expand All @@ -158,8 +160,8 @@ one_direct_presence_sent(Config) ->
end,
[{xmppPresenceSent, 1 + user_alpha(2)},
{xmppPresenceReceived, 1 + user_alpha(2)},
{xmppStanzaSent, 1 + user_alpha(2)},
{xmppStanzaReceived, 1 + user_alpha(2)}]).
{xmppStanzaSent, 1 + user_alpha(6)},
{xmppStanzaReceived, 1 + user_alpha(6)}]).

one_iq_sent(Config) ->
instrumented_story
Expand All @@ -169,11 +171,11 @@ one_iq_sent(Config) ->
escalus_client:send(User1, RosterIq),
escalus_client:wait_for_stanza(User1)
end,
[{xmppIqSent, 1},
{xmppIqReceived, 1},
[{xmppIqSent, 3},
{xmppIqReceived, 3},
{modRosterGets, 1},
{xmppStanzaSent, 1 + user_alpha(1)},
{xmppStanzaReceived, 1 + user_alpha(1)}]).
{xmppStanzaSent, 1 + user_alpha(3)},
{xmppStanzaReceived, 1 + user_alpha(3)}]).

one_message_error(Config) ->
instrumented_story
Expand Down Expand Up @@ -221,11 +223,13 @@ session_counters(Config) ->
escalus:story
(Config, [{alice, 2}, {bob, 1}],
fun(_User11, _User12, _User2) ->
%% Force update
lists:foreach(fun metrics_helper:sample/1, Names),
?assertEqual(3, fetch_global_gauge_value(totalSessionCount, Config)),
?assertEqual(2, fetch_global_gauge_value(uniqueSessionCount, Config)),
?assertEqual(3, fetch_global_gauge_value(nodeSessionCount, Config))
%% Force update
lists:foreach(fun metrics_helper:sample/1, Names),
timer:sleep(timer:seconds(1)),

?assertEqual(3, fetch_global_gauge_value(totalSessionCount, Config)),
?assertEqual(2, fetch_global_gauge_value(uniqueSessionCount, Config)),
?assertEqual(3, fetch_global_gauge_value(nodeSessionCount, Config))
end).

node_uptime(Config) ->
Expand Down Expand Up @@ -352,7 +356,7 @@ user_alpha(NumberOfUsers) ->
instrumented_story(Config, UsersSpecs, StoryFun, CounterSpecs) ->
Befores = fetch_all(Config, CounterSpecs),
StoryResult = escalus:story(Config, UsersSpecs, StoryFun),
Afters = fetch_all(Config, CounterSpecs),
Afters = fetch_all(Config, CounterSpecs),
[ assert_counter_inc(Name, N, find(Name, Befores), find(Name, Afters))
|| {Name, N} <- CounterSpecs ],
StoryResult.
Expand Down
1 change: 0 additions & 1 deletion big_tests/tests/metrics_c2s_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,6 @@ error_presence(Config) ->
Presence = escalus_stanza:presence_direct(escalus_client:short_jid(Alice),
<<"error">>, [ErrorElt]),
escalus:send(Bob, Presence),
escalus:wait_for_stanza(Alice),

wait_for_counter(Errors + 1, xmppErrorPresence)

Expand Down
6 changes: 3 additions & 3 deletions doc/operation-and-maintenance/MongooseIM-metrics.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ MongooseIM uses [ESL's fork of this project](https://github.com/esl/exometer/tre
All metrics are divided into the following groups:

* Per host type metrics: Gathered separately for every host type supported by the cluster.

!!! Warning
If a cluster supports many (thousands or more) host types, performance issues might occur.
To avoid this, use global equivalents of the metrics with `all_metrics_are_global` config option.

* Hook metrics.
They are created for every [hook](../developers-guide/Hooks-and-handlers.md) and incremented on every call to it.

Expand Down Expand Up @@ -137,7 +137,7 @@ As a result it makes more sense to maintain a list of the most relevant or usefu
| `[HostType, xmppMessageReceived]` | spiral | A message is sent to a client. |
| `[HostType, xmppPresenceReceived]` | spiral | A presence is sent to a client. |
| `[HostType, xmppStanzaReceived]` | spiral | A stanza is sent to a client. |
| `[HostType, xmppStanzaCount]` | spiral | A stanza is sent to a client. |
| `[HostType, xmppStanzaCount]` | spiral | A stanza is sent to and by a client. |
| `[HostType, xmppStanzaDropped]` | spiral | A stanza is dropped due to an AMP rule or a `filter_packet` processing flow. |

### Extension-specific metrics
Expand Down
8 changes: 6 additions & 2 deletions src/c2s/mongoose_c2s.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ activate_socket(#c2s_data{socket = Socket}) ->

-spec send_text(c2s_data(), iodata()) -> ok | {error, term()}.
send_text(#c2s_data{socket = Socket}, Text) ->
mongoose_metrics:update(global, [data, xmpp, sent, xml_stanza_size], size(Text)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Careful, because size/1 will only take a tuple or a binary, but exml:to_iolist/1 might return here an iolist of binaries if a list of #xmlel{} records was given. Ideally, this should use iolist_size/1 instead. Will also (theoretically) give better type information.

mongoose_c2s_socket:send_text(Socket, Text).

-spec filter_mechanism(c2s_data(), binary()) -> boolean().
Expand Down Expand Up @@ -705,13 +706,16 @@ handle_state_result(StateData0, C2SState, MaybeAcc,
maybe_send_xml(StateData2, MaybeAcc, MaybeSocketSend),
{next_state, NextFsmState, StateData2, MaybeActions}.

-spec maybe_send_xml(c2s_data(), mongoose_acc:t(), [exml:element()]) -> ok.
maybe_send_xml(_StateData, _Acc, []) ->
ok;
maybe_send_xml(StateData = #c2s_data{host_type = HostType, lserver = LServer}, undefined, ToSend) ->
Acc = mongoose_acc:new(#{host_type => HostType, lserver => LServer, location => ?LOCATION}),
send_element(StateData, ToSend, Acc);
[send_element(StateData, El, Acc) || El <- ToSend],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here we're losing an optimisation. We could call once per packet exml:to_iolist/1, which will generate a single binary, and then gen_tcp:send/X, which will do one socket operation, as many times as there are packets available; or, we could do a single call to exml:to_iolist/1 with the list of records, which will return a iolist list of binaries, and do a single socket operation with that iolist.

Considering that encoding and socket operations are more expensive than metrics, I'd keep them in their most optimal usage and have metrics either do iolist_size/1, which will perfectly free anyway, or, have the metrics code explore the list of binaries and do the update per metrics.

ok;
maybe_send_xml(StateData, Acc, ToSend) ->
send_element(StateData, ToSend, Acc).
[send_element(StateData, El, Acc) || El <- ToSend],
ok.

-spec maybe_deliver(c2s_data(), gen_hook:hook_fn_ret(mongoose_acc:t())) -> mongoose_acc:t().
maybe_deliver(StateData, {ok, Acc}) ->
Expand Down
2 changes: 1 addition & 1 deletion src/ejabberd_sm.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ terminate_session(Pid, Reason) ->
%% Hook handlers
%%====================================================================

-spec node_cleanup(Acc, Args, Extra) -> {ok, Acc} when
-spec node_cleanup(Acc, Args, Extra) -> {ok, Acc} when
Acc :: any(),
Args :: #{node := node()},
Extra :: map().
Expand Down
5 changes: 2 additions & 3 deletions src/metrics/mongoose_metrics.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,8 @@ create_global_metrics() ->
-spec init_predefined_host_type_metrics(mongooseim:host_type()) -> ok.
init_predefined_host_type_metrics(HostType) ->
create_metrics(HostType),
Hooks = mongoose_metrics_hooks:get_hooks(HostType),
ejabberd_hooks:add(Hooks),
ok.
ejabberd_hooks:add(mongoose_metrics_hooks:hooks(HostType)),
gen_hook:add_handlers(mongoose_metrics_hooks:c2s_hooks(HostType)).

init_subscriptions() ->
Reporters = exometer_report:list_reporters(),
Expand Down
131 changes: 71 additions & 60 deletions src/metrics/mongoose_metrics_hooks.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,17 @@
-include("mongoose.hrl").
-include("jlib.hrl").

-export([get_hooks/1]).
-export([hooks/1]).
-export([c2s_hooks/1]).

%%-------------------
%% Internal exports
%%-------------------
-export([sm_register_connection_hook/5,
sm_remove_connection_hook/5,
auth_failed/3,
user_send_packet/4,
user_receive_packet/5,
user_send_packet/3,
user_open_session/3,
Comment on lines +23 to +24
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember these changes well because we will have conflicts in the future with the hook reworks 😛

xmpp_bounce_message/1,
xmpp_stanza_dropped/4,
xmpp_send_element/2,
Expand All @@ -40,34 +41,37 @@
privacy_list_push/5, register_user/3, remove_user/3, roster_get/2,
roster_in_subscription/5, roster_push/4, roster_set/4,
sm_register_connection_hook/5, sm_remove_connection_hook/5,
user_receive_packet/5, user_send_packet/4, xmpp_bounce_message/1,
xmpp_send_element/2, xmpp_stanza_dropped/4]).
user_send_packet/3, user_open_session/3,
xmpp_bounce_message/1, xmpp_send_element/2, xmpp_stanza_dropped/4]).

%%-------------------
%% Implementation
%%-------------------

%% @doc Here will be declared which hooks should be registered
-spec get_hooks(_) -> [ejabberd_hooks:hook(), ...].
get_hooks(HostType) ->
[ {sm_register_connection_hook, HostType, ?MODULE, sm_register_connection_hook, 50},
{sm_remove_connection_hook, HostType, ?MODULE, sm_remove_connection_hook, 50},
{auth_failed, HostType, ?MODULE, auth_failed, 50},
{user_send_packet, HostType, ?MODULE, user_send_packet, 50},
{user_receive_packet, HostType, ?MODULE, user_receive_packet, 50},
{xmpp_stanza_dropped, HostType, ?MODULE, xmpp_stanza_dropped, 50},
{xmpp_bounce_message, HostType, ?MODULE, xmpp_bounce_message, 50},
{xmpp_send_element, HostType, ?MODULE, xmpp_send_element, 50},
{roster_get, HostType, ?MODULE, roster_get, 55},
{roster_set, HostType, ?MODULE, roster_set, 50},
{roster_push, HostType, ?MODULE, roster_push, 50},
{roster_in_subscription, HostType, ?MODULE, roster_in_subscription, 55},
{register_user, HostType, ?MODULE, register_user, 50},
{remove_user, HostType, ?MODULE, remove_user, 50},
{privacy_iq_get, HostType, ?MODULE, privacy_iq_get, 1},
{privacy_iq_set, HostType, ?MODULE, privacy_iq_set, 1},
{privacy_check_packet, HostType, ?MODULE, privacy_check_packet, 55},
{sm_broadcast, HostType, ?MODULE, privacy_list_push, 1}].
-spec hooks(mongooseim:host_type()) -> [ejabberd_hooks:hook()].
hooks(HostType) ->
[{sm_register_connection_hook, HostType, ?MODULE, sm_register_connection_hook, 50},
{sm_remove_connection_hook, HostType, ?MODULE, sm_remove_connection_hook, 50},
{auth_failed, HostType, ?MODULE, auth_failed, 50},
{xmpp_stanza_dropped, HostType, ?MODULE, xmpp_stanza_dropped, 50},
{xmpp_bounce_message, HostType, ?MODULE, xmpp_bounce_message, 50},
{xmpp_send_element, HostType, ?MODULE, xmpp_send_element, 50},
{roster_get, HostType, ?MODULE, roster_get, 55},
{roster_set, HostType, ?MODULE, roster_set, 50},
{roster_push, HostType, ?MODULE, roster_push, 50},
{roster_in_subscription, HostType, ?MODULE, roster_in_subscription, 55},
{register_user, HostType, ?MODULE, register_user, 50},
{remove_user, HostType, ?MODULE, remove_user, 50},
{privacy_iq_get, HostType, ?MODULE, privacy_iq_get, 1},
{privacy_iq_set, HostType, ?MODULE, privacy_iq_set, 1},
{privacy_check_packet, HostType, ?MODULE, privacy_check_packet, 55},
{sm_broadcast, HostType, ?MODULE, privacy_list_push, 1}].

-spec c2s_hooks(mongooseim:host_type()) -> gen_hook:hook_list(mongoose_c2s_hooks:hook_fn()).
c2s_hooks(HostType) ->
[{user_send_packet, HostType, fun ?MODULE:user_send_packet/3, #{}, 50},
{user_open_session, HostType, fun ?MODULE:user_open_session/3, #{}, 50}].

-spec sm_register_connection_hook(any(), mongooseim:host_type(), tuple(), jid:jid(), term()
) -> any().
Expand All @@ -92,13 +96,14 @@ auth_failed(Acc, _, Server) ->
mongoose_metrics:update(HostType, sessionAuthFails, 1),
Acc.

-spec user_send_packet(mongoose_acc:t(), jid:jid(), tuple(), tuple()
) -> mongoose_acc:t().
user_send_packet(Acc, _, _, Packet) ->
HostType = mongoose_acc:host_type(Acc),
-spec user_send_packet(mongoose_acc:t(), mongoose_c2s:hook_params(), map()) ->
mongoose_c2s_hooks:hook_result().
user_send_packet(Acc, _Params, #{host_type := HostType}) ->
mongoose_metrics:update(HostType, xmppStanzaSent, 1),
user_send_packet_type(HostType, Packet),
Acc.
mongoose_metrics:update(HostType, xmppStanzaCount, 1),
El = mongoose_acc:element(Acc),
user_send_packet_type(HostType, El),
{ok, Acc}.

-spec user_send_packet_type(HostType :: mongooseim:host_type(),
Packet :: exml:element()) -> ok | {error, not_found}.
Expand All @@ -109,23 +114,6 @@ user_send_packet_type(HostType, #xmlel{name = <<"iq">>}) ->
user_send_packet_type(HostType, #xmlel{name = <<"presence">>}) ->
mongoose_metrics:update(HostType, xmppPresenceSent, 1).

-spec user_receive_packet(mongoose_acc:t(), jid:jid(), tuple(), tuple(), tuple()
) -> mongoose_acc:t().
user_receive_packet(Acc, _, _, _, Packet) ->
HostType = mongoose_acc:host_type(Acc),
mongoose_metrics:update(HostType, xmppStanzaReceived, 1),
user_receive_packet_type(HostType, Packet),
Acc.

-spec user_receive_packet_type(HostType :: mongooseim:host_type(),
Packet :: exml:element()) -> ok | {error, not_found}.
user_receive_packet_type(HostType, #xmlel{name = <<"message">>}) ->
mongoose_metrics:update(HostType, xmppMessageReceived, 1);
user_receive_packet_type(HostType, #xmlel{name = <<"iq">>}) ->
mongoose_metrics:update(HostType, xmppIqReceived, 1);
user_receive_packet_type(HostType, #xmlel{name = <<"presence">>}) ->
mongoose_metrics:update(HostType, xmppPresenceReceived, 1).

-spec xmpp_bounce_message(Acc :: mongoose_acc:t()) -> mongoose_acc:t().
xmpp_bounce_message(Acc) ->
HostType = mongoose_acc:host_type(Acc),
Expand All @@ -139,25 +127,48 @@ xmpp_stanza_dropped(Acc, _, _, _) ->
mongoose_metrics:update(HostType, xmppStanzaDropped, 1),
Acc.

-spec xmpp_send_element(Acc :: mongoose_acc:t(), Server :: jid:server()) -> mongoose_acc:t().
xmpp_send_element(Acc, _El) ->
-spec xmpp_send_element(Acc :: mongoose_acc:t(), El :: exml:element()) -> mongoose_acc:t().
xmpp_send_element(Acc, #xmlel{name = Name} = El)
when Name == <<"iq">>; Name == <<"message">>; Name == <<"presence">> ->
HostType = mongoose_acc:host_type(Acc),
mongoose_metrics:update(HostType, xmppStanzaCount, 1),
case mongoose_acc:stanza_type(Acc) of
case exml_query:attr(El, <<"type">>) of
<<"error">> ->
mongoose_metrics:update(HostType, xmppErrorTotal, 1),
case mongoose_acc:stanza_name(Acc) of
<<"iq">> ->
mongoose_metrics:update(HostType, xmppErrorIq, 1);
<<"message">> ->
mongoose_metrics:update(HostType, xmppErrorMessage, 1);
<<"presence">> ->
mongoose_metrics:update(HostType, xmppErrorPresence, 1)
end;
_ -> ok
xmpp_send_element_error(HostType, El);
_ ->
mongoose_metrics:update(HostType, xmppStanzaReceived, 1),
xmpp_send_element_type(HostType, El)
end,
Acc;
xmpp_send_element(Acc, _El) ->
Acc.

-spec xmpp_send_element_error(HostType :: mongooseim:host_type(),
Packet :: exml:element()) -> ok | {error, not_found}.
xmpp_send_element_error(HostType, #xmlel{name = <<"message">>}) ->
mongoose_metrics:update(HostType, xmppErrorMessage, 1);
xmpp_send_element_error(HostType, #xmlel{name = <<"iq">>}) ->
mongoose_metrics:update(HostType, xmppErrorIq, 1);
xmpp_send_element_error(HostType, #xmlel{name = <<"presence">>}) ->
mongoose_metrics:update(HostType, xmppErrorPresence, 1).
NelsonVides marked this conversation as resolved.
Show resolved Hide resolved

-spec xmpp_send_element_type(HostType :: mongooseim:host_type(),
Packet :: exml:element()) -> ok | {error, not_found}.
xmpp_send_element_type(HostType, #xmlel{name = <<"message">>}) ->
mongoose_metrics:update(HostType, xmppMessageReceived, 1);
xmpp_send_element_type(HostType, #xmlel{name = <<"iq">>}) ->
mongoose_metrics:update(HostType, xmppIqReceived, 1);
xmpp_send_element_type(HostType, #xmlel{name = <<"presence">>}) ->
mongoose_metrics:update(HostType, xmppPresenceReceived, 1).

-spec user_open_session(mongoose_acc:t(), mongoose_c2s:hook_params(), map()) ->
mongoose_c2s_hooks:hook_result().
user_open_session(Acc, _Params, #{host_type := HostType}) ->
mongoose_metrics:update(HostType, xmppStanzaSent, 1),
mongoose_metrics:update(HostType, xmppStanzaCount, 1),
mongoose_metrics:update(HostType, xmppIqSent, 1),
{ok, Acc}.

%% Roster

Expand Down