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.

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

C2s/metrics #3800

Merged
merged 2 commits into from
Oct 11, 2022
Merged

C2s/metrics #3800

merged 2 commits into from
Oct 11, 2022

Conversation

kamilwaz
Copy link

@kamilwaz kamilwaz commented Oct 7, 2022

Fixes metrics suites.

The following changes were introduced:

  • moved counting received stanzas from user_receive_packet to xmpp_send_element as first one can be dropped by further hooks and not send at all,
  • fixed the numbers in test cases (I checked the real numbers with mongoose_c2s:send_text/2 and mongoose_c2s:handle_socket_data/2),
  • converted hooks to c2s hooks,
  • fixed the documention for xmppStanzaCount metric,
  • xmpp_send_element hook processes one element per run instead of not all.

@kamilwaz kamilwaz changed the base branch from master to feature/mongoose_c2s October 7, 2022 12:19
@mongoose-im

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Base: 69.81% // Head: 70.34% // Increases project coverage by +0.52% 🎉

Coverage data is based on head (a5c97ac) compared to base (636aa93).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@                   Coverage Diff                    @@
##           feature/mongoose_c2s    #3800      +/-   ##
========================================================
+ Coverage                 69.81%   70.34%   +0.52%     
========================================================
  Files                       536      536              
  Lines                     34872    34879       +7     
========================================================
+ Hits                      24346    24535     +189     
+ Misses                    10526    10344     -182     
Impacted Files Coverage Δ
src/ejabberd_sm.erl 83.01% <ø> (+0.96%) ⬆️
src/c2s/mongoose_c2s.erl 68.79% <100.00%> (+0.32%) ⬆️
src/metrics/mongoose_metrics.erl 90.27% <100.00%> (+3.71%) ⬆️
src/metrics/mongoose_metrics_hooks.erl 97.72% <100.00%> (+0.10%) ⬆️
src/pubsub/node_pep.erl 68.51% <0.00%> (-5.56%) ⬇️
src/async_pools/mongoose_aggregator_worker.erl 63.33% <0.00%> (-5.01%) ⬇️
src/domain/mongoose_domain_loader.erl 89.28% <0.00%> (-3.58%) ⬇️
src/mam/mod_mam_muc_rdbms_arch_async.erl 82.85% <0.00%> (-2.86%) ⬇️
src/inbox/mod_inbox_rdbms_async.erl 72.05% <0.00%> (-1.48%) ⬇️
src/mongoose_acc.erl 90.54% <0.00%> (-1.36%) ⬇️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Oct 10, 2022

small_tests_24 / small_tests / a8e3477
Reports root / small


small_tests_25 / small_tests / a8e3477
Reports root / small


ldap_mnesia_24 / ldap_mnesia / a8e3477
Reports root/ big
OK: 1500 / Failed: 0 / User-skipped: 686 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / a8e3477
Reports root/ big
OK: 1500 / Failed: 0 / User-skipped: 686 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / a8e3477
Reports root/ big
OK: 3499 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / a8e3477
Reports root/ big
OK: 3675 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / a8e3477
Reports root/ big
OK: 1589 / Failed: 0 / User-skipped: 597 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / a8e3477
Reports root/ big
OK: 3498 / Failed: 1 / User-skipped: 78 / Auto-skipped: 0

inbox_SUITE:async_pools:one_to_one:msg_sent_to_offline_user
{error,
  {{assertion_failed,assert,is_iq_result,
     {xmlel,<<"message">>,
       [{<<"from">>,
         <<"alice_msg_sent_to_offline_user_1296@domain.example.com/res1">>},
        {<<"to">>,
         <<"bob_msg_sent_to_offline_user_1296@domain.example.com/res1">>},
        {<<"type">>,<<"chat">>}],
       [{xmlel,<<"body">>,[],[{xmlcdata,<<"test">>}]},
        {xmlel,<<"stanza-id">>,
          [{<<"by">>,
          <<"bob_msg_sent_to_offline_user_1296@domain.example.com">>},
           {<<"id">>,<<"BQLCCIEJF202">>},
           {<<"xmlns">>,<<"urn:xmpp:sid:0">>}],
          []}]},
     "<message from='alice_msg_sent_to_offline_user_1296@domain.example.com/res1' to='bob_msg_sent_to_offline_user_1296@domain.example.com/res1' type='chat'><body>test</body><stanza-id by='bob_msg_sent_to_offline_user_1296@domain.example.com' id='BQLCCIEJF202' xmlns='urn:xmpp:sid:0'/></message>"},
   [{escalus_new_assert,assert_true,2,
      [{file,
         "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
       {line,84}]},
    {escalus_session,session,1,
      [{file,
         "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_session.erl"},
       {line,131}]},
    {escalus_session,session,2,
      [{file,
         "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_session.erl"},
       {line,273}]},
    {escalus_connection,connection_step,2,
      [{file,
         "/home/circleci/project/big_tests/_build/def...

Report log


dynamic_domains_mysql_redis_25 / mysql_redis / a8e3477
Reports root/ big
OK: 3473 / Failed: 0 / User-skipped: 104 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / a8e3477
Reports root/ big
OK: 3675 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / a8e3477
Reports root/ big
OK: 3497 / Failed: 1 / User-skipped: 78 / Auto-skipped: 1

last_SUITE:valid_queries:last_offline_user
{error,
  {{assertion_failed,assert,is_last_result,
     {xmlel,<<"presence">>,
       [{<<"from">>,<<"alice_unnamed_1418@domain.example.com/res1">>},
        {<<"to">>,<<"alice_unnamed_1418@domain.example.com/res1">>},
        {<<"type">>,<<"unavailable">>}],
       [{xmlel,<<"status">>,[],[{xmlcdata,<<"Unknown condition">>}]}]},
     "<presence from='alice_unnamed_1418@domain.example.com/res1' to='alice_unnamed_1418@domain.example.com/res1' type='unavailable'><status>Unknown condition</status></presence>"},
   [{escalus_new_assert,assert_true,2,
      [{file,
         "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
       {line,84}]},
    {last_SUITE,'-last_offline_user/1-fun-0-',2,
      [{file,"/home/circleci/project/big_tests/tests/last_SUITE.erl"},
       {line,133}]},
    {escalus_story,story,4,
      [{file,
         "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
       {line,72}]},
    {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
    {test_server,run_test_case_eval1,6,
      [{file,"test_server.erl"},{line,1291}]},
    {test_server,run_test_case_eval,9,
      [{file,"test_server.erl"},{line,1223}]}]}}

Report log


mysql_redis_25 / mysql_redis / a8e3477
Reports root/ big
OK: 3664 / Failed: 1 / User-skipped: 103 / Auto-skipped: 0

mod_event_pusher_rabbit_SUITE:group_chat_message_publish:group_chat_message_received_event_properly_formatted
{error,
  {{assertMatch,
     [{module,mod_event_pusher_rabbit_SUITE},
      {line,422},
      {expression,
        "get_decoded_message_from_rabbit ( AliceGroupChatMsgRecvRK )"},
      {pattern,
        "# { << \"from_user_id\" >> := BobRoomJID , << \"to_user_id\" >> := AliceFullJID , << \"message\" >> := Message }"},
      {value,
        #{<<"from_user_id">> => <<"muc_publish@muc.localhost">>,
        <<"message">> => <<>>,
        <<"to_user_id">> =>
          <<"alice_unnamed_2033@localhost/res1">>}}]},
   [{mod_event_pusher_rabbit_SUITE,
      '-group_chat_message_received_event_properly_formatted/1-fun-1-',3,
      [{file,
         "/home/circleci/project/big_tests/tests/mod_event_pusher_rabbit_SUITE.erl"},
       {line,422}]},
    {escalus_story,story,4,
      [{file,
         "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
       {line,72}]},
    {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
    {test_server,run_test_case_eval1,6,
      [{file,"test_server.erl"},{line,1291}]},
    {test_server,run_test_case_eval,9,
      [{file,"test_server.erl"},{line,1223}]}]}}

Report log


mssql_mnesia_25 / odbc_mssql_mnesia / a8e3477
Reports root/ big
OK: 3674 / Failed: 1 / User-skipped: 89 / Auto-skipped: 0

service_domain_db_SUITE:db:db_get_all_dynamic
{error,
  {{badrpc,timeout},
   [{distributed_helper,rpc,
      [#{node => mongooseim@localhost},service_domain_db,sync_local,[]],
      [{file,
         "/home/circleci/project/big_tests/tests/distributed_helper.erl"},
       {line,117}]},
    {service_domain_db_SUITE,sync_local,1,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,1175}]},
    {service_domain_db_SUITE,sync,0,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,1154}]},
    {service_domain_db_SUITE,db_get_all_dynamic,1,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,354}]},
    {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
    {test_server,run_test_case_eval1,6,
      [{file,"test_server.erl"},{line,1291}]},
    {test_server,run_test_case_eval,9,
      [{file,"test_server.erl"},{line,1223}]}]}}

Report log


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / a8e3477
Reports root/ big
OK: 3499 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / a8e3477
Reports root/ big
OK: 3498 / Failed: 1 / User-skipped: 78 / Auto-skipped: 0

rest_client_SUITE:roster:add_and_remove_some_contacts_properly
{error,{{badmatch,{{<<"500">>,<<"Internal Server Error">>},<<>>}},
    [{rest_client_SUITE,add_contact_check_roster_push,2,
              [{file,"/home/circleci/project/big_tests/tests/rest_client_SUITE.erl"},
               {line,1304}]},
     {lists,foreach_1,2,[{file,"lists.erl"},{line,1442}]},
     {rest_client_SUITE,'-add_and_remove_some_contacts_properly/1-fun-1-',
              4,
              [{file,"/home/circleci/project/big_tests/tests/rest_client_SUITE.erl"},
               {line,1241}]},
     {escalus_story,story,4,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,72}]},
     {rest_client_SUITE,add_and_remove_some_contacts_properly,1,
              [{file,"/home/circleci/project/big_tests/tests/rest_client_SUITE.erl"},
               {line,1236}]},
     {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
     {test_server,run_test_case_eval1,6,
            [{file,"test_server.erl"},{line,1291}]},
     {test_server,run_test_case_eval,9,
            [{file,"test_server.erl"},{line,1223}]}]}}

Report log


mssql_mnesia_25 / odbc_mssql_mnesia / a8e3477
Reports root/ big
OK: 3675 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / a8e3477
Reports root/ big
OK: 3498 / Failed: 1 / User-skipped: 78 / Auto-skipped: 0

inbox_extensions_SUITE:async_pools:one_to_one:mute_muted_entry_gets_unmuted
{error,
  {{assert,
     [{module,inbox_extensions_SUITE},
      {line,798},
      {expression,"escalus_pred : is_message ( Message )"},
      {expected,true},
      {value,false}]},
   [{inbox_extensions_SUITE,check_message_with_properties,4,
      [{file,
         "/home/circleci/project/big_tests/tests/inbox_extensions_SUITE.erl"},
       {line,798}]},
    {inbox_extensions_SUITE,set_inbox_properties,4,
      [{file,
         "/home/circleci/project/big_tests/tests/inbox_extensions_SUITE.erl"},
       {line,792}]},
    {inbox_extensions_SUITE,'-mute_muted_entry_gets_unmuted/1-fun-2-',2,
      [{file,
         "/home/circleci/project/big_tests/tests/inbox_extensions_SUITE.erl"},
       {line,568}]},
    {escalus_story,story,4,
      [{file,
         "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
       {line,72}]},
    {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
    {test_server,run_test_case_eval1,6,
      [{file,"test_server.erl"},{line,1291}]},
    {test_server,run_test_case_eval,9,
      [{file,"test_server.erl"},{line,1223}]}]}}

Report log


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / a8e3477
Reports root/ big
OK: 3499 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0

@kamilwaz kamilwaz marked this pull request as ready for review October 10, 2022 12:29
Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

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

Few relevant comments below

Comment on lines +23 to +24
user_send_packet/3,
user_open_session/3,
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 😛

@@ -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.

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.

src/metrics/mongoose_metrics_hooks.erl Show resolved Hide resolved
@mongoose-im
Copy link
Collaborator

mongoose-im commented Oct 11, 2022

small_tests_24 / small_tests / a5c97ac
Reports root / small


small_tests_25 / small_tests / a5c97ac
Reports root / small


ldap_mnesia_24 / ldap_mnesia / a5c97ac
Reports root/ big
OK: 1500 / Failed: 0 / User-skipped: 686 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / a5c97ac
Reports root/ big
OK: 3499 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / a5c97ac
Reports root/ big
OK: 1500 / Failed: 0 / User-skipped: 686 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / a5c97ac
Reports root/ big
OK: 3675 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / a5c97ac
Reports root/ big
OK: 1589 / Failed: 0 / User-skipped: 597 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / a5c97ac
Reports root/ big
OK: 3499 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0


elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / a5c97ac
Reports root/ big
OK: 1923 / Failed: 0 / User-skipped: 598 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / a5c97ac
Reports root/ big
OK: 3675 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / a5c97ac
Reports root/ big
OK: 3473 / Failed: 0 / User-skipped: 104 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / a5c97ac
Reports root/ big
OK: 3661 / Failed: 0 / User-skipped: 103 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / a5c97ac
Reports root/ big
OK: 1763 / Failed: 0 / User-skipped: 590 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / a5c97ac
Reports root/ big
OK: 3675 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / a5c97ac
Reports root/ big
OK: 3499 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0

Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

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

Excellent 👍🏽

@NelsonVides NelsonVides merged commit e3a74ae into feature/mongoose_c2s Oct 11, 2022
@NelsonVides NelsonVides deleted the c2s/metrics branch October 11, 2022 11:33
@jacekwegr jacekwegr added this to the 6.1.0 milestone Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants