Start only used metrics #3096
Merged
Start only used metrics #3096
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #3096 +/- ##
==========================================
- Coverage 79.08% 79.07% -0.02%
==========================================
Files 386 386
Lines 31829 31824 -5
==========================================
- Hits 25172 25164 -8
- Misses 6657 6660 +3
Continue to review full report at Codecov.
|
gustawlippa
force-pushed
the
used-metrics
branch
2 times, most recently
from
8a20453
to
0a340f8
Compare
chrzaszcz
reviewed
Apr 23, 2021
gustawlippa
force-pushed
the
used-metrics
branch
from
9ea2274
to
d0c19c9
Compare
Now mod_mam is responsible for starting and updating metrics related to MAM. Thanks to this change MAM metrics will not be reported if Mongoose is started with MAM disabled.
These are unused and replaced by the definitions in global_distrib_metrics.hrl.
It is updated in the module, so it could at least exist.
gustawlippa
force-pushed
the
used-metrics
branch
from
3819614
to
7a43626
Compare
chrzaszcz
approved these changes
Apr 28, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the problem of starting metrics for not used modules in MIM which is a performance concern and bad user experience.
Most of the culprits are metrics in the file
mongoose_metrics_definitions, which are custom hook metrics (they should override the automatic hook metrics). Their handlers, which implement custom logic for updating the metrics, are present in filesmongoose_metrics_hooksandmongoose_metrics_mam_hooks.The proposition is to move the place of starting the metrics and registering these custom hooks back to the modules which these metrics concern, while keeping the files with hook code as is, at least for now.
There are some problems left but would require metrics name changes or even a rework of some metrics, which could be breaking changes for the users.
I'm not sure how to approach
modPrivacy*metrics and would appreciate feedback.There is an issue right now that
mongoose_metricscontains a list of hooks that don't have generic metrics and have some custom logic to calculate metrics (often weirdly named) based on them (this logic is for example inmongoose_metrics_hooks).This list can lead to inconsistencies, and the custom logic may sometimes be too coupled with the modules.
My proposition would be to get rid of this list, unify the hook metrics and keep the automatic metrics for all of them (for example with a
hooksprefix).The custom logic to calculate more complex metrics could maybe be than moved from files like
mongoose_metrics_hooksto the modules which evokeensure_metricsor similar functions on startup, so the places which are "responsible" for the metrics.mongoose_metrics_hooksand similar would keep some very general metrics like the xmpp ones.Other solution could just be to check the metrics ensured based on
mongoose_metrics_definitionsagainst the configured modules.Metrics that are run for MIM configured without any modules:
Changed by this PR:
mod_csi.?GLOBAL_DISTRIB_MESSAGES_SENTinglobal_metrics.hrlmod_mam, like the ones below.mod_mam- now will start inmod_mam_muc. AddedmodMucMamDroppedIQbecause it was being updated and didn't exist.Problematic/other comments - for now unchanged:
ejabberd_auth:timed_call/4.ejabberd_local. Some hooks and their metrics have the_hooksuffix, other not, maybe this could be unified.mongoose_metrics_mam_hooksas an automatic hook, I think it was supposed to be filtered bymongoose_metrics:filter_hook/1and is the same asmodMamLookupsbut left as is.mod_mamones is that the hook is called fromejabberd_c2s- can these be sent withoutmod_privacyon? If so, maybe it should stay on by default? If this is not a concern should be moved to start withmod_privacy. Should anyway be replaced by a generic hook metric in my opinion.mod_registerbut just all registered users. I would just delete these (maybe add a different metric formod_registeritself) and keep the automatic hook metric instead of this.modPrivacy*is run from different places in the code even ifmod_rosteris not enabled.modRegisterCount.Good ones:
modPrivacyPushmetric is sm_broadcast times sessionCounts per Host.