-
Notifications
You must be signed in to change notification settings - Fork 428
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
Instrument mod_ping
#4288
Instrument mod_ping
#4288
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/instrument #4288 +/- ##
======================================================
- Coverage 84.75% 84.75% -0.01%
======================================================
Files 556 556
Lines 33888 33884 -4
======================================================
- Hits 28723 28717 -6
- Misses 5165 5167 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good in general, but I think we should consistently remove all mongoose_metricd
checks form our tests. That was one of the intentions of the instrumentation rework - by removing the dependency on exometer, we would be able to switch it off in the future.
This comment was marked as outdated.
This comment was marked as outdated.
1c615da
to
809e558
Compare
This comment was marked as outdated.
This comment was marked as outdated.
809e558
to
b9d35aa
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.
8dc8416
to
2e7b823
Compare
elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / 2e7b823 small_tests_25 / small_tests / 2e7b823 small_tests_26_arm64 / small_tests / 2e7b823 small_tests_26 / small_tests / 2e7b823 ldap_mnesia_25 / ldap_mnesia / 2e7b823 bosh_SUITE:essential:accept_higher_hold_value{error,
{{assertEqual,
[{module,bosh_SUITE},
{line,265},
{expression,"get_bosh_sessions ( )"},
{expected,[]},
{value,
[{bosh_session,<<"52c0fb22073c6506d39992ab4610d43b9ea1413e">>,
<9341.10203.0>}]}]},
[{bosh_SUITE,accept_higher_hold_value,1,
[{file,"/home/circleci/project/big_tests/tests/bosh_SUITE.erl"},
{line,265}]},
{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}]}]}} dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 2e7b823 dynamic_domains_mysql_redis_26 / mysql_redis / 2e7b823 ldap_mnesia_26 / ldap_mnesia / 2e7b823 jingle_SUITE:all:resp_4xx_from_sip_proxy_results_in_session_terminate{error,
{{assertion_failed,assert,is_iq_result,
{xmlel,<<"iq">>,
[{<<"from">>,<<"error.480@localhost">>},
{<<"to">>,
<<"alice_resp_4xx_from_sip_proxy_results_in_session_terminate_1051@localhost/res1">>},
{<<"id">>,<<"840f2bde-2ae5-4057-abe0-35e4ce5c5fb6">>},
{<<"type">>,<<"set">>}],
[{xmlel,<<"jingle">>,
[{<<"xmlns">>,<<"urn:xmpp:jingle:1">>},
{<<"action">>,<<"session-terminate">>},
{<<"sid">>,<<"6078c59e-67f3-4456-bdf0-fd8202f180c4">>}],
[{xmlel,<<"reason">>,[],
[{xmlel,<<"general-error">>,[],[]},
{xmlel,<<"sip-error">>,
[{<<"code">>,<<"480">>}],
[{xmlcdata,<<"Temporarily Unavailable">>}]}]}]}]},
"<iq from='error.480@localhost' to='alice_resp_4xx_from_sip_proxy_results_in_session_terminate_1051@localhost/res1' id='840f2bde-2ae5-4057-abe0-35e4ce5c5fb6' type='set'><jingle xmlns='urn:xmpp:jingle:1' action='session-terminate' sid='6078c59e-67f3-4456-bdf0-fd8202f180c4'><reason><general-error/><sip-error code='480'>Temporarily Unavailable</sip-error></reason></jingle></iq>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{jingle_SUITE,send_initiate_and_wait_for_first_iq_set,2,
[{file,"/home/circleci/project/big_tests/tests/jingle_SUITE.erl"},
{line,390}]},
{jingle_SUITE,
'-resp_... internal_mnesia_26 / internal_mnesia / 2e7b823 dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / 2e7b823 dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / 2e7b823 pgsql_cets_26 / pgsql_cets / 2e7b823 pgsql_mnesia_25 / pgsql_mnesia / 2e7b823 pgsql_mnesia_26 / pgsql_mnesia / 2e7b823 mysql_redis_26 / mysql_redis / 2e7b823 mssql_mnesia_26 / odbc_mssql_mnesia / 2e7b823 ldap_mnesia_25 / ldap_mnesia / 2e7b823 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
This PR adds instrumentation to
mod_ping
.One important change is the merging of two events (
ping_response_time
andping_response
) into one, as they were always invoked simultaneously.The measurement of time in
mod_ping_response
does not usemongoose_instrument:span
because the time measurement is done through hooks.