Instrument/metric improvements #4249
Conversation
|
elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / a682886 small_tests_26 / small_tests / a682886 small_tests_25 / small_tests / a682886 small_tests_26_arm64 / small_tests / a682886 ldap_mnesia_25 / ldap_mnesia / a682886 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / a682886 dynamic_domains_mysql_redis_26 / mysql_redis / a682886 ldap_mnesia_26 / ldap_mnesia / a682886 dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / a682886 internal_mnesia_26 / internal_mnesia / a682886 pgsql_cets_26 / pgsql_cets / a682886 dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / a682886 mysql_redis_26 / mysql_redis / a682886 carboncopy_SUITE:one2one:dropped_client_doesnt_create_duplicate_carbons{error,
{{badmatch,
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_dropped_client_doesnt_create_duplicate_carbons_586@localhost">>},
{<<"to">>,
<<"alice_dropped_client_doesnt_create_duplicate_carbons_586@localhost/res2">>},
{<<"xmlns">>,<<"jabber:client">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"sent">>,
[{<<"xmlns">>,<<"urn:xmpp:carbons:2">>}],
[{xmlel,<<"forwarded">>,
[{<<"xmlns">>,<<"urn:xmpp:forward:0">>}],
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_dropped_client_doesnt_create_duplicate_carbons_586@localhost/res1">>},
{<<"type">>,<<"chat">>},
{<<"to">>,
<<"bob_dropped_client_doesnt_create_duplicate_carbons_586@localhost/res1">>},
{<<"xmlns">>,<<"jabber:client">>}],
[{xmlel,<<"body">>,[],
[{xmlcdata,
<<"And pious action">>}]}]}]}]}]}]},
[{carboncopy_SUITE,
'-dropped_client_doesnt_create_duplicate_carbons/1-fun-0-',4,
[{file,
"/home/circleci/project/big_tests/tests/carboncopy_SUITE.erl"},
{line,189}]},
{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,1793}]},
{test_server,run_test_case_eval1,6,
[{file,"test_serv...pgsql_mnesia_25 / pgsql_mnesia / a682886 pgsql_mnesia_26 / pgsql_mnesia / a682886 mssql_mnesia_26 / odbc_mssql_mnesia / a682886 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/instrument #4249 +/- ##
======================================================
+ Coverage 84.23% 84.51% +0.27%
======================================================
Files 556 556
Lines 33660 33695 +35
======================================================
+ Hits 28354 28476 +122
+ Misses 5306 5219 -87 ☔ View full report in Codecov by Sentry. |
- Failing handlers shouldn't cause the instrumented code to crash - Subsequent handlers should still be executed - There is no removal of broken handlers (like telemetry), because that would put the system in an inconsistent state if a handler fails intermittently.
The goal is to allow automatic initialisation/cleanup.
chrzaszcz
force-pushed
the
instrument/metric-improvements
branch
from
a682886
to
fb71715
Compare
|
elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / fb71715 small_tests_25 / small_tests / fb71715 small_tests_26 / small_tests / fb71715 small_tests_26_arm64 / small_tests / fb71715 ldap_mnesia_25 / ldap_mnesia / fb71715 ldap_mnesia_26 / ldap_mnesia / fb71715 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / fb71715 dynamic_domains_mysql_redis_26 / mysql_redis / fb71715 carboncopy_SUITE:one2one:dropped_client_doesnt_create_duplicate_carbons{error,
{{badmatch,
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_dropped_client_doesnt_create_duplicate_carbons_586@domain.example.com">>},
{<<"to">>,
<<"alice_dropped_client_doesnt_create_duplicate_carbons_586@domain.example.com/res2">>},
{<<"xmlns">>,<<"jabber:client">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"sent">>,
[{<<"xmlns">>,<<"urn:xmpp:carbons:2">>}],
[{xmlel,<<"forwarded">>,
[{<<"xmlns">>,<<"urn:xmpp:forward:0">>}],
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_dropped_client_doesnt_create_duplicate_carbons_586@domain.example.com/res1">>},
{<<"type">>,<<"chat">>},
{<<"to">>,
<<"bob_dropped_client_doesnt_create_duplicate_carbons_586@domain.example.com/res1">>},
{<<"xmlns">>,<<"jabber:client">>}],
[{xmlel,<<"body">>,[],
[{xmlcdata,
<<"And pious action">>}]}]}]}]}]}]},
[{carboncopy_SUITE,
'-dropped_client_doesnt_create_duplicate_carbons/1-fun-0-',4,
[{file,
"/home/circleci/project/big_tests/tests/carboncopy_SUITE.erl"},
{line,189}]},
{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,1793}]},
{test_server,run_test_c...dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / fb71715 internal_mnesia_26 / internal_mnesia / fb71715 pgsql_mnesia_25 / pgsql_mnesia / fb71715 dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / fb71715 pgsql_cets_26 / pgsql_cets / fb71715 mysql_redis_26 / mysql_redis / fb71715 pgsql_mnesia_26 / pgsql_mnesia / fb71715 mssql_mnesia_26 / odbc_mssql_mnesia / fb71715 |
This way start and stop operations are more organised, and the functionality is tested.
- The goal is to use them in the handler modules. - label_key and label_value are a bit repetitive, but this way the specs are as strict as possible. If the list grows longer, we could make the type more generic instead of enumerating all values.
1. Support the 'all_hosts_are_global' option 2. Move prefix management from mongoose_metrics to eliminate the dependency on legacy code. 3. Reset already existing metrics on setup. The goal is to have fresh repeatable metric state if you restart mongoose_instrument in tests. There is no metric removal, because it doesn't seem to be needed.
More initialisation is needed because of host-type prefix handling.
1. Add type specs. 2. Use strings as metric names to avoid calling 'list_to_atom'. This doesn't seem to have a big performance penalty. 3. Reset already existing metrics on startup, just like for exometer. 4. Initialise counters with zero - previous initial value was 'undefined', resulting in a delay in initial rate metric calculation in Prometheus.
Metric name is now a string, not an atom.
chrzaszcz
force-pushed
the
instrument/metric-improvements
branch
from
fb71715
to
b768315
Compare
|
elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / b768315 small_tests_25 / small_tests / b768315 small_tests_26 / small_tests / b768315 small_tests_26_arm64 / small_tests / b768315 ldap_mnesia_25 / ldap_mnesia / b768315 ldap_mnesia_26 / ldap_mnesia / b768315 dynamic_domains_mysql_redis_26 / mysql_redis / b768315 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / b768315 internal_mnesia_26 / internal_mnesia / b768315 pgsql_mnesia_25 / pgsql_mnesia / b768315 pgsql_cets_26 / pgsql_cets / b768315 pgsql_mnesia_26 / pgsql_mnesia / b768315 dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / b768315 mysql_redis_26 / mysql_redis / b768315 mssql_mnesia_26 / odbc_mssql_mnesia / b768315 |
| full_metric_name(EventName, MetricName) -> | ||
| list_to_atom(atom_to_list(EventName) ++ "_" ++ atom_to_list(MetricName)). | ||
| atom_to_list(EventName) ++ "_" ++ atom_to_list(MetricName). |
There was a problem hiding this comment.
This in comparison to the previous code is pretty much having the same performance, true, but, here on every metric update we're concatenating the same two lists again and again. Is this necessary? Maybe the metric name can be [EventName, MetricName]? Or some similar structure that implies no copying? Just asking out loud, I don't know the required API for the prometheus library here.
There was a problem hiding this comment.
Prometheus library accepts only atoms or flat strings. Any changes would require modifying prometheus.erl.
There was a problem hiding this comment.
That's just how Prometheus metric naming works - so for me it makes sense that the library doesn't give an illusion of supporting lists.
This PR groups several improvements of metric handling in
mongoose_instrument.Changes:
all_metrics_are_globaloption ininstrumentation.exometer.set_up.start/0andstop/0in themongoose_instrumentbehaviour.list_to_atom.mongoose_metrics.