-
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 global distrib #4292
Instrument/mod global distrib #4292
Conversation
92d0038
to
f0e977b
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.
3a98440
to
b9f8e89
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.
3536c3e
to
5073d6d
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.
d3e3b5b
to
c9c982f
Compare
From testing locally, it seems to be more stable here - not sure why.
Fixes a race condition in the test. Maybe it could be fixed by retrying bounce with a small timeout after sending all the messages, but I didn't try this and went with the known solution from the other test.
Metrics configuration has been moved up in the supervision tree, as now it's not differentiated by hosts in the metric name. The hosts are part of the measurement.
c9c982f
to
694a879
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.
The issue was uncovered by the small tests. Additionally, the instrumentation is started correctly in these tests. The host_type is a misnomer, as global_distrib does not work for host types - this issue may be revisited. The other issue is that other events in global_distrib are without any labels, as they were run with `global`, or can be differentiated by the host value in the measurements map.
694a879
to
503e95b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Also removes `ensure_subscribed_metric/3` as this was the last place in which it was called.
Also move instrumentation asserts to more correct places.
The name of measurement is appended to the event name in metrics, so now the names will make more sense.
503e95b
to
1a93b6b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / fc3690f small_tests_25 / small_tests / fc3690f small_tests_26 / small_tests / fc3690f small_tests_26_arm64 / small_tests / fc3690f ldap_mnesia_25 / ldap_mnesia / fc3690f ldap_mnesia_26 / ldap_mnesia / fc3690f dynamic_domains_mysql_redis_26 / mysql_redis / fc3690f dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / fc3690f dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / fc3690f bosh_SUITE:essential_https:accept_higher_hold_value{error,
{{assertEqual,
[{module,bosh_SUITE},
{line,265},
{expression,"get_bosh_sessions ( )"},
{expected,[]},
{value,
[{bosh_session,<<"8c68b2f228cb2c6bcc9e13182b04366da253b12e">>,
<10231.10677.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,1793}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1302}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1234}]}]}} dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / fc3690f pgsql_cets_26 / pgsql_cets / fc3690f internal_mnesia_26 / internal_mnesia / fc3690f pgsql_mnesia_25 / pgsql_mnesia / fc3690f pgsql_mnesia_26 / pgsql_mnesia / fc3690f mysql_redis_26 / mysql_redis / fc3690f mssql_mnesia_26 / odbc_mssql_mnesia / fc3690f dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / fc3690f |
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 to me
This PR instruments all
mod_global_distrib
modules.There were some issues when implementing, mainly with tests being unstable.
The other issue is that
mod_global_distrib
can be started for multiple hosts, but doesn't support host types. Some of the metrics used to be named by the host in which context they were running. Now, they havehost
as one of the measurements, and I decided against introducing a new label so it won't be confused withhost_type
, but this is something we could consider. Because of that, some detail is lost in the metrics - now all are effectively global.GLOBAL_DISTRIB_INCOMING_ERRORED
andGLOBAL_DISTRIB_OUTGOING_ERRORED
metrics were not tested, and since I spent a lot of time on this PR I decided to leave it as it was.