Skip to content
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

Metrics per host type #3120

Merged
merged 9 commits into from
May 31, 2021
Merged

Metrics per host type #3120

merged 9 commits into from
May 31, 2021

Conversation

gustawlippa
Copy link
Contributor

@gustawlippa gustawlippa commented May 17, 2021

This PR updates the metrics to use host types instead of hosts. There are still some places left that use host types and should be updated when the modules are updated (for example mongoose_wpool_rdbms).

Because the host type may be any string, the spaces " " are changed to underscores "_" in the metrics modules in a simple manner on every call. Because of that, when calling functions like mongoose_metrics:get_metric_value/1, which expect the metric name, one has to provide a sanitized name - this is not needed when calling the metrics API expecting host types.

There are also some small fixes, like a bugfix for a test suite that occurred seemingly only on my computer, and getting rid of the "Failing the test due failed rerun groups" message that was printed even if the tests were successful.

@gustawlippa gustawlippa changed the title Fix metrics not being reported. Metrics per host type May 24, 2021
The testcase could fail if the common_test init process died too early and the
port was closed. It did not manifest in our CI, but I noticed it during
developement.
"Failing the test due to failed rerun groups" will not be printed everytime now.
@gustawlippa gustawlippa force-pushed the host-type-metrics branch 2 times, most recently from 75ceff5 to 4b829e6 Compare May 27, 2021 09:10
@esl esl deleted a comment from codecov bot May 27, 2021
@codecov
Copy link

codecov bot commented May 27, 2021

Codecov Report

Merging #3120 (25f0555) into master (5166ab3) will increase coverage by 0.01%.
The diff coverage is 96.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3120      +/-   ##
==========================================
+ Coverage   79.46%   79.48%   +0.01%     
==========================================
  Files         395      395              
  Lines       32168    32189      +21     
==========================================
+ Hits        25563    25584      +21     
  Misses       6605     6605              
Impacted Files Coverage Δ
src/ejabberd_local.erl 77.16% <ø> (-0.36%) ⬇️
src/global_distrib/mod_global_distrib_utils.erl 65.42% <0.00%> (-0.62%) ⬇️
src/metrics/mongoose_metrics_hooks.erl 95.29% <94.91%> (-1.77%) ⬇️
src/metrics/mongoose_api_metrics.erl 92.15% <100.00%> (ø)
src/metrics/mongoose_metrics.erl 96.10% <100.00%> (+0.13%) ⬆️
src/ejabberd_sm.erl 84.17% <0.00%> (-0.34%) ⬇️
src/mam/mod_mam_utils.erl 89.56% <0.00%> (-0.34%) ⬇️
src/mod_muc_room.erl 77.45% <0.00%> (-0.06%) ⬇️
src/mod_muc_log.erl 77.72% <0.00%> (ø)
src/pubsub/mod_pubsub_db_mnesia.erl 92.82% <0.00%> (+0.42%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5166ab3...25f0555. Read the comment docs.

@gustawlippa gustawlippa marked this pull request as ready for review May 27, 2021 11:05
Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All seems good to me, but I'm not confident about merging this one, yet, so maybe let's see about more opinions @chrzaszcz @DenysGonchar

doc/rest-api/Metrics-backend.md Show resolved Hide resolved
Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, mostly minor comments from me. Some hooks needs fixing soon as they are not converted to host types yet.

I would like to see a test suite for metrics added to dynamic_domains.spec. Would it be possible to do it now or not yet?

big_tests/tests/metrics_helper.erl Outdated Show resolved Hide resolved
big_tests/tests/metrics_helper.erl Outdated Show resolved Hide resolved
src/metrics/mongoose_metrics_hooks.erl Show resolved Hide resolved
test/carbon_cache_server.erl Outdated Show resolved Hide resolved
gustawlippa and others added 2 commits May 28, 2021 15:53
Co-authored-by: Paweł Chrząszcz <pawel.chrzaszcz@erlang-solutions.com>
@chrzaszcz chrzaszcz merged commit cf515ef into master May 31, 2021
@chrzaszcz chrzaszcz deleted the host-type-metrics branch May 31, 2021 09:50
@arcusfelis
Copy link
Contributor


=== Ended at 2021-05-31 14:05:25
=== Location: [{escalus_mongooseim,metric_type,100},
              {escalus_mongooseim,read_metric_initial_value,60},
              {lists,foldl,1263},
              {escalus_mongooseim,maybe_read_initial_metric_values,38},
              {escalus_story,story,68},
              {test_server,ts_tc,1754},
              {test_server,run_test_case_eval1,1263},
              {test_server,run_test_case_eval,1195}]
=== === Reason: no match of right hand side value []
  in function  escalus_mongooseim:metric_type/1 (/Users/mikhailuvarov/erlang/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl, line 100)
  in call from escalus_mongooseim:read_metric_initial_value/2 (/Users/mikhailuvarov/erlang/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl, line 60)
  in call from lists:foldl/3 (lists.erl, line 1263)
  in call from escalus_mongooseim:maybe_read_initial_metric_values/1 (/Users/mikhailuvarov/erlang/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl, line 38)
  in call from escalus_story:story/4 (/Users/mikhailuvarov/erlang/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_story.erl, line 68)
  in call from test_server:ts_tc/3 (test_server.erl, line 1754)
  in call from test_server:run_test_case_eval1/6 (test_server.erl, line 1263)
  in call from test_server:run_test_case_eval/9 (test_server.erl, line 1195)

@DenysGonchar
Copy link
Collaborator


=== Ended at 2021-05-31 14:05:25
=== Location: [{escalus_mongooseim,metric_type,100},
              {escalus_mongooseim,read_metric_initial_value,60},
              {lists,foldl,1263},
              {escalus_mongooseim,maybe_read_initial_metric_values,38},
              {escalus_story,story,68},
              {test_server,ts_tc,1754},
              {test_server,run_test_case_eval1,1263},
              {test_server,run_test_case_eval,1195}]
=== === Reason: no match of right hand side value []
  in function  escalus_mongooseim:metric_type/1 (/Users/mikhailuvarov/erlang/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl, line 100)
  in call from escalus_mongooseim:read_metric_initial_value/2 (/Users/mikhailuvarov/erlang/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl, line 60)
  in call from lists:foldl/3 (lists.erl, line 1263)
  in call from escalus_mongooseim:maybe_read_initial_metric_values/1 (/Users/mikhailuvarov/erlang/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl, line 38)
  in call from escalus_story:story/4 (/Users/mikhailuvarov/erlang/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_story.erl, line 68)
  in call from test_server:ts_tc/3 (test_server.erl, line 1754)
  in call from test_server:run_test_case_eval1/6 (test_server.erl, line 1263)
  in call from test_server:run_test_case_eval/9 (test_server.erl, line 1195)

are you sure that the config provided to escalus:story/3 contains host type specific metrics? I had the similar problem in mod_ping_SUITE, see changes for mongoose_metrics parameter in this PR

@Premwoik Premwoik added this to the 5.0.0 milestone Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants