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

Global metrics only switch #940

Merged
merged 9 commits into from
Sep 6, 2016
Merged

Conversation

fenek
Copy link
Member

@fenek fenek commented Aug 29, 2016

This PR adds all_metrics_are_global switch.

Proposed changes include:

  • mongoose_metrics got some serious refactoring.
  • New option, when enabled, merges all per-host metrics into global equivalents, effectively reducing CPU and memory footprint in clusters with lots of XMPP domains supported.
  • A side effect is an unification of metrics names. Host part (or global atom) always comes first now.
  • Only unit tests and metrics_api suite are updated, because remaining metrics test cases rely heavily on RPC, which doesn't offer nice and consistent way of making calls to more than one node. The new switch is enabled on dev node 2 to avoid restarts or introducing yet another test configuration
  • Updated documentation, including a note about performance problem the new switch solves.

Not all metrics test suites are modified to verify both nodes (with switch enabled and disabled) because they rely on many RPC calls and this mechanism is not ready yet for flexible calls to any node.

@fenek fenek added the WIP 🚧 label Aug 29, 2016
@fenek fenek force-pushed the global-metrics-only-switch branch from 7e534d1 to 567a88e Compare August 29, 2016 14:00
@fenek fenek force-pushed the global-metrics-only-switch branch 3 times, most recently from 232b9b1 to 66eaa8e Compare September 3, 2016 16:13
make_global_groups(Groups) ->
[{make_global_group_name(GN), Opts, Cases} || {GN, Opts, Cases} <- Groups].

userspec(User1Count, Config) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should accept the same user specs as escalus:story/3 to make things simpler.
And BTW, why exactly do we need this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose of this function is to return escalus:story/3-compatible userspec, based on presence (or absence) of all_metrics_are_global flag in test config. When it's present, users from node 2 are used in spec. This makes this choice transparent for actual suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see it now, it's not the easiest thing to follow. From what I understand it assumes that the escalus_users in Config contains only correct users. Probably a word of explanation will make things easier for others.

Copy link
Member Author

Choose a reason for hiding this comment

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

Word of explanation like explaining it in a comment to this function, right? No problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's what I meant :)

@michalwski michalwski merged commit 21edb10 into esl:master Sep 6, 2016
@michalwski michalwski mentioned this pull request Nov 8, 2016
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.

3 participants