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: exometer report #4326

Merged
merged 10 commits into from
Jul 24, 2024
Merged

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Jul 19, 2024

The goal is to configure exometer reporters in mongooseim.toml instead of app.config, and to use mongoose_instrument instead of the obsolete mongoose_metrics.

In TOML, instrumentation.exometer.report.graphite is a list of sections, e.g.:

[[instrumentation.exometer.report.graphite]]
  host = "localhost"
  interval = 15_000
  prefix = "mim"

So far only graphite is supported, but others like statsd can be added later if necessary.
After processing, the resulting [instrumentation, exometer, report] option is map with auto-generated reporter names as keys. Such format allows easy mapping to the Exometer reporter names.

The example configuration mentioned above results in the following map:

#{instrumentation =>
  #{exometer =>
    #{report =>
      #{'graphite:localhost:2003' =>
        #{module => exometer_report_graphite,
          port => 2003,
          host => "localhost",
          prefix => "mim",
          interval => 15000,
          connect_timeout => 5000,
          api_key => ""}},
      all_metrics_are_global => false}}}

Accepted options are same as before, and their defaults are as they were in the default app.config.

Other changes

  • Add the config options as an argument to the start and stop callbacks in mongoose_instrument.
  • Wait for linked processes in async_helper. Otherwise, if you start Module:start_link(...) with async_helper, the helper wouldn't wait for it to terminate when calling stop. As a result, subsequent tests might fail to initialize if they use Module as well.

Notes

  • exometer_* libraries are forked under esl, and they got quite old when compared with upstream. We could consider updating them.
  • The api_key option has the "" default, but it could be optional for consistency. This would require a small modification in exometer_report_graphite though. We could do it separately, but for me it's very minor.
  • Documentation is excluded from this PR on purpose, as the docs page doesn't exist yet. We will do it in the upcoming docs task.
  • The code fails if the reporters cannot be set up. This seems ok for me, because networking issues don't cause such failures. We can change this later.
  • Removal of metrics and subscriptions is not implemented yet, and we can do it in a separate task. It's not a major issue.

@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz force-pushed the instrument/exometer_report branch from 704bfe5 to ba9fb25 Compare July 22, 2024 15:05
@chrzaszcz chrzaszcz changed the title WIP Instrument: exometer report Jul 22, 2024
@chrzaszcz chrzaszcz force-pushed the instrument/exometer_report branch from ba9fb25 to 9e6f551 Compare July 22, 2024 15:06
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz force-pushed the instrument/exometer_report branch from 9e6f551 to 1825f7d Compare July 23, 2024 07:48
@mongoose-im

This comment was marked as outdated.

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.43%. Comparing base (d9eed9d) to head (2b08cc0).

Additional details and impacted files
@@                 Coverage Diff                 @@
##           feature/instrument    #4326   +/-   ##
===================================================
  Coverage               84.43%   84.43%           
===================================================
  Files                     551      551           
  Lines                   33854    33853    -1     
===================================================
+ Hits                    28583    28585    +2     
+ Misses                   5271     5268    -3     

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

@chrzaszcz chrzaszcz force-pushed the instrument/exometer_report branch from 1825f7d to 7abfc5c Compare July 23, 2024 11:51
@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz force-pushed the instrument/exometer_report branch 3 times, most recently from 41505dd to 6176728 Compare July 23, 2024 14:55
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz force-pushed the instrument/exometer_report branch from 6176728 to e8b78af Compare July 23, 2024 16:37
@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz force-pushed the instrument/exometer_report branch from e8b78af to 905c3ee Compare July 24, 2024 06:53
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

This way the options can be passed directly to the modules.
Config options:
  - default values for 'connect_timeout', 'interval' and 'port' are
    same as before;
  - 'prefix' and 'env_prefix' are not defined by default;
  - 'api_key' has the default value of '' because it is mandatory
    in exometer_report_graphite;
  - 'host' is mandatory.

Config format:
  - In TOML, 'instrumentation.exometer.report.graphite' is a list
  - After processing, [instrumentation, exometer, report] becomes a map
    with auto-generated reporter names as keys. Such format allows
    easy mapping to the Exometer reporter names.

In the current implementation, failing to start is an unrecoverable
error, but this shouldn't happen in typical cases, e.g. with
connectivity issues.
They are handled by mongoose_instrument now
Otherwise, if you start Module:start_link(...) with async_helper,
the helper wouldn't wait for it to terminate when calling 'stop'.
As a result subsequent tests might fail to initialize if they use
Module as well.
- Add API: start, stop, subscribe, get_metric
- Unsubscription is automatic when the subscriber goes down
- Test more options including prefixes
- Check both global and host-type metrics
@chrzaszcz chrzaszcz force-pushed the instrument/exometer_report branch from 6e44198 to 2b08cc0 Compare July 24, 2024 09:40
@mongoose-im
Copy link
Collaborator

mongoose-im commented Jul 24, 2024

elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / 2b08cc0
Reports root/ big
OK: 457 / Failed: 0 / User-skipped: 45 / Auto-skipped: 0


small_tests_25 / small_tests / 2b08cc0
Reports root / small


small_tests_26 / small_tests / 2b08cc0
Reports root / small


small_tests_26_arm64 / small_tests / 2b08cc0
Reports root / small


ldap_mnesia_25 / ldap_mnesia / 2b08cc0
Reports root/ big
OK: 2341 / Failed: 0 / User-skipped: 910 / Auto-skipped: 0


ldap_mnesia_26 / ldap_mnesia / 2b08cc0
Reports root/ big
OK: 2341 / Failed: 0 / User-skipped: 910 / Auto-skipped: 0


dynamic_domains_mysql_redis_26 / mysql_redis / 2b08cc0
Reports root/ big
OK: 4656 / Failed: 0 / User-skipped: 139 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 2b08cc0
Reports root/ big
OK: 4689 / Failed: 0 / User-skipped: 106 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / 2b08cc0
Reports root/ big
OK: 4689 / Failed: 0 / User-skipped: 106 / Auto-skipped: 0


internal_mnesia_26 / internal_mnesia / 2b08cc0
Reports root/ big
OK: 2483 / Failed: 0 / User-skipped: 768 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / 2b08cc0
Reports root/ big
OK: 4686 / Failed: 0 / User-skipped: 109 / Auto-skipped: 0


pgsql_cets_26 / pgsql_cets / 2b08cc0
Reports root/ big
OK: 4582 / Failed: 0 / User-skipped: 177 / Auto-skipped: 0


mysql_redis_26 / mysql_redis / 2b08cc0
Reports root/ big
OK: 5061 / Failed: 0 / User-skipped: 134 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 2b08cc0
Reports root/ big
OK: 5080 / Failed: 0 / User-skipped: 115 / Auto-skipped: 0


mssql_mnesia_26 / odbc_mssql_mnesia / 2b08cc0
Reports root/ big
OK: 5077 / Failed: 0 / User-skipped: 118 / Auto-skipped: 0


pgsql_mnesia_26 / pgsql_mnesia / 2b08cc0
Reports root/ big
OK: 5080 / Failed: 0 / User-skipped: 115 / Auto-skipped: 0

@chrzaszcz chrzaszcz marked this pull request as ready for review July 24, 2024 10:16
Copy link
Contributor

@gustawlippa gustawlippa 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 to me, I have no comments 👍

@gustawlippa gustawlippa merged commit b07999b into feature/instrument Jul 24, 2024
4 checks passed
@gustawlippa gustawlippa deleted the instrument/exometer_report branch July 24, 2024 11:46
@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