Change how reporters subscribe to metrics #3148
Draft
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
Now every call to ensure_metric/3 also subscribes all reporters to it. This makes init_subscriptions/0 redundant. The benefit is that some metrics started after the call to mongoose_metrics:init were not reported and now such situation is impossible.
Codecov Report
@@ Coverage Diff @@
## master #3148 +/- ##
==========================================
- Coverage 79.52% 79.48% -0.04%
==========================================
Files 397 397
Lines 32320 32310 -10
==========================================
- Hits 25701 25681 -20
- Misses 6619 6629 +10
Continue to review full report at Codecov.
|
arcusfelis
reviewed
Oct 18, 2021
| subscribe_metric(Name, Type) -> | ||
| Reporters = exometer_report:list_reporters(), | ||
| lists:foreach( | ||
| fun({ReporterName, _ReporterPid}) -> |
There was a problem hiding this comment.
good catch to match Reporter here properly.
Reporting a bad result from exometer_report:subscribe would help to avoid this small bug...
arcusfelis
reviewed
Oct 18, 2021
| exometer_report:subscribe(Reporter, Name, default, Interval). | ||
|
|
||
| subscribe_to_all(Reporter, Interval) -> |
There was a problem hiding this comment.
there are two reasons it's done as is:
- it could be a bit faster to subscribe to all at once (need to test that)
- metrics are reported with different initial offset interval if not subscribed at once.
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.
Now every call to
ensure_metric/3also subscribes all reporters to it, which I think is a less surprising API. This makesinit_subscriptions/0redundant. The benefit is that some metrics started after the call to mongoose_metrics:init were not reported (global.unregister_command), thus not visible in Graphite for example, and now such situation is impossible.This PR is just a proposition to solve the problem. The upside is that if you ensure a metric exists, it is also reported (if any reporters are set). The downside is that there is less flexibility in adding reporters, or for example using some reporters for some metrics, and not subscribing them to others.