-
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
add metric: type of config file #2918
Conversation
8ae79ca
to
0f4363c
Compare
8674.1 / Erlang 23.0.3 / small_tests / 5260e00 8674.2 / Erlang 23.0.3 / internal_mnesia / 5260e00 8674.3 / Erlang 23.0.3 / odbc_mssql_mnesia / 5260e00 8674.4 / Erlang 23.0.3 / mysql_redis / 5260e00 8674.5 / Erlang 23.0.3 / riak_mnesia / 5260e00 8674.6 / Erlang 23.0.3 / ldap_mnesia / 5260e00 8674.7 / Erlang 23.0.3 / elasticsearch_and_cassandra_mnesia / 5260e00 8674.9 / Erlang 22.3 / pgsql_mnesia / 5260e00 mod_global_distrib_SUITE:cluster_restart:test_location_disconnect{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,
<<"alicE_test_location_disconnect_70.604476@localhost/res1">>,
escalus_tcp,<0.9356.2>,
[{event_manager,<0.9352.2>},
{server,<<"localhost">>},
{username,<<"alicE_test_location_disconnect_70.604476">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.9352.2>},
{server,<<"localhost">>},
{username,
<<"alicE_test_location_disconnect_70.604476">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,<<"alicE_test_location_disconnect_70.604476">>},
{server,<<"localhost">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,<<"alicE_test_location_disconnect_70.604476">>},
{server,<<"localhost">>},
{password,<<"matygrysa">>},
{stream_id,<<"32645200072fb112">>}]},
5000],
[{file,
"/home/travis/build/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{mod_global_distrib_SUITE,'-test_location_disconnect/1-fun-0-',2,
[{file,
"/home/travis/build/esl/MongooseIM/big_tests/tests/mod_global_distrib_SUITE.erl"},
{line,621}]},
{escalus_story,story,4,
[{file,
... |
Codecov Report
@@ Coverage Diff @@
## master #2918 +/- ##
==========================================
- Coverage 79.16% 79.13% -0.03%
==========================================
Files 378 378
Lines 32798 32804 +6
==========================================
- Hits 25965 25960 -5
- Misses 6833 6844 +11
Continue to review full report at Codecov.
|
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.
I have a few comments.
ConfigType = case filename:extension(ConfigPath) of | ||
".toml" -> toml; | ||
".cfg" -> cfg; | ||
_ -> unknown_config_type |
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.
This is not reachable - mongoose_config_parser:parse_file
is called before and it would crash.
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.
Sure, I just wanted to be double cautious just in case. If you think line 225 is unnecessary, I'll have it removed.
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.
The case expression in mongoose_config_parser:parse_file
has only two clauses and I think these two should be consistent as they are checking the same thing. Services are started after the config is parsed, so MongooseIM would (and should) crash before this line is executed. Reaching this line would mean a bug in the code.
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.
I hoped we could remove that line, but it is minor, so I am proceeding with the merge.
@@ -313,6 +296,9 @@ xmpp_stanzas_counts_are_reported(Config) -> | |||
mongoose_helper:wait_until(fun xmpp_messages_count_is_reported/0, true), | |||
mongoose_helper:wait_until(fun xmpp_stanzas_counts_are_reported/0, true). | |||
|
|||
config_type_is_reported(_Config) -> | |||
mongoose_helper:wait_until(fun config_type_is_reported/0, true). |
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.
I think it would be good to check that the correct format is reported here.
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.
Sure, we could add this check. Should we expect "toml" to be returned, or should we check the file format? If we were to check the file format I think I would use mongoose_system_metrics_collector:get_config_type/0
. With such test we only test if the metric is reported, we don't check if it is collected correctly, as we get the information in the exact same way, which puts in question checking the format at all.
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.
Actually this is going to be a short-lived metric, I think that checking it manually for toml
and cfg
should be enough. The code can remain unchanged.
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.
There was no answer here, so I tested it myself and it works correctly.
0f4363c
to
b64df30
Compare
8676.1 / Erlang 23.0.3 / small_tests / 548f9f5 8676.2 / Erlang 23.0.3 / internal_mnesia / 548f9f5 8676.4 / Erlang 23.0.3 / mysql_redis / 548f9f5 8676.3 / Erlang 23.0.3 / odbc_mssql_mnesia / 548f9f5 8676.7 / Erlang 23.0.3 / elasticsearch_and_cassandra_mnesia / 548f9f5 8676.5 / Erlang 23.0.3 / riak_mnesia / 548f9f5 8676.6 / Erlang 23.0.3 / ldap_mnesia / 548f9f5 8676.9 / Erlang 22.3 / pgsql_mnesia / 548f9f5 |
This PR adds a metric to see what type of configuration file is being used. The aim is to determine the adoption of the new config file format.