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

Instrument/hooks #4289

Merged
merged 9 commits into from
Jun 4, 2024
Merged

Instrument/hooks #4289

merged 9 commits into from
Jun 4, 2024

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented May 29, 2024

The main goal is to use mongoose_instrument instead of mongoose_metrics for the automatic hook metrics.

Main changes:

  • gen_hook is using a new mongoose_instrument_hooks module to set up, tear down and execute events.
  • Events are named hook_HOOKNAME, e.g. hook_filter_packet.
  • Host type is a label, and this implies that if you try to set up both global and host-type-specific handlers for the same hook, it would result in an error. Such an error is caught and logged, but the handlers are set up anyway (as they were before). We could introduce a notion of scope defined statically per hook (global or host-type), but it would be a separate task - and a big one.
  • The logic for skipping hooks is same as before, with the exception of mam*_flush_messages hooks, which are removed, because they were used only for instrumentation, and provided no value, because these operations are already instrumented.

Other minor changes are described in commit messages.

@chrzaszcz chrzaszcz changed the base branch from master to feature/instrument May 29, 2024 14:17
@mongoose-im
Copy link
Collaborator

mongoose-im commented May 29, 2024

ldap_mnesia_25 / ldap_mnesia / af5d56e
Reports root


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / af5d56e
Reports root


pgsql_mnesia_25 / pgsql_mnesia / af5d56e
Reports root


pgsql_mnesia_26 / pgsql_mnesia / af5d56e
Reports root


dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / af5d56e
Reports root


pgsql_cets_26 / pgsql_cets / af5d56e
Reports root


dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / af5d56e
Reports root


internal_mnesia_26 / internal_mnesia / af5d56e
Reports root


ldap_mnesia_26 / ldap_mnesia / af5d56e
Reports root


mysql_redis_26 / mysql_redis / af5d56e
Reports root


mssql_mnesia_26 / odbc_mssql_mnesia / af5d56e
Reports root


dynamic_domains_mysql_redis_26 / mysql_redis / af5d56e
Reports root


small_tests_25 / small_tests / af5d56e
Reports root / small


small_tests_26 / small_tests / af5d56e
Reports root / small


small_tests_26_arm64 / small_tests / af5d56e
Reports root / small

@mongoose-im

This comment was marked as outdated.

Copy link

codecov bot commented May 31, 2024

Codecov Report

Attention: Patch coverage is 98.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 84.74%. Comparing base (22fb94f) to head (4c4d89d).
Report is 4 commits behind head on feature/instrument.

Files Patch % Lines
src/instrument/mongoose_instrument_hooks.erl 98.14% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           feature/instrument    #4289      +/-   ##
======================================================
+ Coverage               84.52%   84.74%   +0.21%     
======================================================
  Files                     556      557       +1     
  Lines                   33899    33908       +9     
======================================================
+ Hits                    28653    28735      +82     
+ Misses                   5246     5173      -73     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

Move the mongoose_metrics:filter_hook/2 logic to is_instrumented/2
A few tests checked that the same hook could have both
global and host-type handlers. This is not the intended behaviour, and
results in an error log.

One extra test was added for this case, while the remaining tests were
amended to test the usual case: a hook is either global or
per-host-type (not both).
Remove all_metrics_are_global when it is no longer needed.
Config needs to exist until mongoose_instrument is stopped.
Hooks are instrumented, and instrumentation needs a real host type

Btw, using host types that are not configured could cause issues in
the future, but fixing this would require a lot of changes.
These hooks were used only for instrumentation,
and they are redundant, because mongoose_instrument has flush events
with the same data.
Also, the MUC hook was not instrumented for some reason.
@mongoose-im
Copy link
Collaborator

mongoose-im commented Jun 3, 2024

elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / 4c4d89d
Reports root/ big
OK: 457 / Failed: 0 / User-skipped: 41 / Auto-skipped: 0


small_tests_25 / small_tests / 4c4d89d
Reports root / small


small_tests_26 / small_tests / 4c4d89d
Reports root / small


small_tests_26_arm64 / small_tests / 4c4d89d
Reports root / small


ldap_mnesia_25 / ldap_mnesia / 4c4d89d
Reports root/ big
OK: 2290 / Failed: 0 / User-skipped: 905 / Auto-skipped: 0


ldap_mnesia_26 / ldap_mnesia / 4c4d89d
Reports root/ big
OK: 2290 / Failed: 0 / User-skipped: 905 / Auto-skipped: 0


dynamic_domains_mysql_redis_26 / mysql_redis / 4c4d89d
Reports root/ big
OK: 4589 / Failed: 0 / User-skipped: 138 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 4c4d89d
Reports root/ big
OK: 4622 / Failed: 0 / User-skipped: 105 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / 4c4d89d
Reports root/ big
OK: 4619 / Failed: 0 / User-skipped: 108 / Auto-skipped: 0


pgsql_cets_26 / pgsql_cets / 4c4d89d
Reports root/ big
OK: 4519 / Failed: 0 / User-skipped: 174 / Auto-skipped: 0


internal_mnesia_26 / internal_mnesia / 4c4d89d
Reports root/ big
OK: 2430 / Failed: 0 / User-skipped: 765 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / 4c4d89d
Reports root/ big
OK: 4622 / Failed: 0 / User-skipped: 105 / Auto-skipped: 0


mysql_redis_26 / mysql_redis / 4c4d89d
Reports root/ big
OK: 4992 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 4c4d89d
Reports root/ big
OK: 5013 / Failed: 0 / User-skipped: 112 / Auto-skipped: 0


pgsql_mnesia_26 / pgsql_mnesia / 4c4d89d
Reports root/ big
OK: 5013 / Failed: 0 / User-skipped: 112 / Auto-skipped: 0


mssql_mnesia_26 / odbc_mssql_mnesia / 4c4d89d
Reports root/ big
OK: 5010 / Failed: 0 / User-skipped: 115 / Auto-skipped: 0

@chrzaszcz chrzaszcz marked this pull request as ready for review June 3, 2024 15:39
Copy link
Contributor

@JanuszJakubiec JanuszJakubiec 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. Thanks for the changes.

@JanuszJakubiec JanuszJakubiec merged commit 1991bca into feature/instrument Jun 4, 2024
4 checks passed
@JanuszJakubiec JanuszJakubiec deleted the instrument/hooks branch June 4, 2024 13:20
@jacekwegr jacekwegr added this to the 6.3.0 milestone Oct 3, 2024
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.

4 participants