Instrument/hook handlers: c2s #4310
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/instrument #4310 +/- ##
======================================================
+ Coverage 84.64% 84.65% +0.01%
======================================================
Files 557 556 -1
Lines 33889 33881 -8
======================================================
- Hits 28685 28682 -3
+ Misses 5204 5199 -5 ☔ View full report in Codecov by Sentry. |
chrzaszcz
force-pushed
the
instrument/hook-handlers-c2s
branch
from
7223590 to
27ea344
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
chrzaszcz
force-pushed
the
instrument/hook-handlers-c2s
branch
from
e31cd35 to
7af893e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
chrzaszcz
force-pushed
the
instrument/hook-handlers-c2s
branch
4 times, most recently
from
61b3cea to
56a0b81
Compare
This comment was marked as outdated.
This comment was marked as outdated.
chrzaszcz
force-pushed
the
instrument/hook-handlers-c2s
branch
from
56a0b81 to
8872fa9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
chrzaszcz
force-pushed
the
instrument/hook-handlers-c2s
branch
from
8872fa9 to
0be83c3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
chrzaszcz
force-pushed
the
instrument/hook-handlers-c2s
branch
from
8bc3652 to
209a526
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
- Report incoming elements (received by the server) as soon as they arrive, i.e. before any filtering or custom handling. - Report outgoing elements (sent by the server) when they are sent, i.e. after any filtering or custom handling. This way, the reported events should not miss any elements from the network traffic.
The system metric names are kept unchanged, because changing them would make it more difficult to analyse statistics from different MIM versions.
chrzaszcz
force-pushed
the
instrument/hook-handlers-c2s
branch
from
9294f87 to
729c5a3
Compare
chrzaszcz
force-pushed
the
instrument/hook-handlers-c2s
branch
from
729c5a3 to
c54f2b4
Compare
This is needed to set up and tear down instrumentation.
They were used only for instrumentation.
Also: reorganize tests to focus on different event types.
Test it with domain_isolation, because it is a typical place where stanzas are dropped.
chrzaszcz
force-pushed
the
instrument/hook-handlers-c2s
branch
from
c54f2b4 to
912ab5c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / 912ab5c small_tests_25 / small_tests / 912ab5c small_tests_26_arm64 / small_tests / 912ab5c small_tests_26 / small_tests / 912ab5c ldap_mnesia_25 / ldap_mnesia / 912ab5c dynamic_domains_mysql_redis_26 / mysql_redis / 912ab5c dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 912ab5c internal_mnesia_26 / internal_mnesia / 912ab5c ldap_mnesia_26 / ldap_mnesia / 912ab5c dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / 912ab5c dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / 912ab5c pgsql_mnesia_25 / pgsql_mnesia / 912ab5c pgsql_cets_26 / pgsql_cets / 912ab5c mysql_redis_26 / mysql_redis / 912ab5c pgsql_mnesia_26 / pgsql_mnesia / 912ab5c mssql_mnesia_26 / odbc_mssql_mnesia / 912ab5c |
chrzaszcz
force-pushed
the
instrument/hook-handlers-c2s
branch
from
912ab5c to
3855cd4
Compare
|
elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / 3855cd4 small_tests_25 / small_tests / 3855cd4 small_tests_26 / small_tests / 3855cd4 small_tests_26_arm64 / small_tests / 3855cd4 ldap_mnesia_25 / ldap_mnesia / 3855cd4 ldap_mnesia_26 / ldap_mnesia / 3855cd4 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 3855cd4 dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / 3855cd4 dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / 3855cd4 internal_mnesia_26 / internal_mnesia / 3855cd4 dynamic_domains_mysql_redis_26 / mysql_redis / 3855cd4 pgsql_mnesia_25 / pgsql_mnesia / 3855cd4 pgsql_cets_26 / pgsql_cets / 3855cd4 mysql_redis_26 / mysql_redis / 3855cd4 pgsql_mnesia_26 / pgsql_mnesia / 3855cd4 mssql_mnesia_26 / odbc_mssql_mnesia / 3855cd4 small_tests_26 / small_tests / 3855cd4 |
| @@ -35,10 +35,12 @@ host_types() -> | |||
| %%-------------------------------------------------------------------- | |||
|
|
|||
| init_per_suite(Config) -> | |||
| instrument_helper:start([{router_stanza_dropped, #{host_type => host_type()}}]), | |||
There was a problem hiding this comment.
Minor, but maybe use the declared events helper for consistency with other test suites?
There was a problem hiding this comment.
I am declaring the event explicitly to avoid the case when adding a new event to the module would cause tests to fail.
The goal is to replace all the remaining hook-based metrics in
mongoose_metrics_hookswith instrumentation.Most of the reworked metrics are for sent and received XML elements. The naming was confusing, e.g.:
xmppIqSent,xmppMessageSent, ... metrics were actually for incoming messages, i.e. received by the server.xmppIqReceived,xmppMessageReceived, ... metrics were actually for outgoing messages, i.e. sent by server. Even in the docs they were described like this:xmppErrorIq,xmppErrorMessage, ... metrics were only for outgoing errors, while clients can send errors as well.Moreover, the metrics were not triggered for all incoming/outgoing elements because of the placement of the hooks in
mongoose_c2sand additional processing/filtering.The c2s metrics are now replaced with two events:
c2s_element_in- emitted as soon as an XML element is received from a client.c2s_element_out- emitted as soon as an XML element is sent to a client.Both events contain measurements (counters) for various stanza types. The
in/outnaming should be also used fors2sandcomponentsin the future - this is why there are no names likeelement_from_client.There are two more events reworked in this PR:
router_stanza_droppedreplaces thexmpp_stanza_droppedhook and thexmppStanzaDroppedmetric.sm_message_bouncedreplaces thexmpp_bounce_messagehook and thexmppMessageBouncedmetric.Their placement is a bit arbitrary - there was no need to move
message_bounced(it was triggered in one module), whilestanza_droppedneeded to be placed in a new module, because it was called from two places (mongoose_local_delivery,mod_amp). This is why I added a helper function for the latter. Alternatively, we could have two separate events (e.g.router_stanza_droppedandamp_stanza_dropped), but I decided to keep it simple for now.System metrics (Analytics) are still using the old stanza event naming, because I didn't want to rename external events to avoid issues with aggregation. We could rework them in a separate story if needed. The translation of event names to metrics was easy.