feat(resource): ask for metrics only when needed #10359

keynslug · 2023-04-10T15:04:29Z

Summary

`🤖 Generated by Copilot at a71ce2e`

This pull request refactors the resource metrics management in the emqx_resource app and introduces a new version of the bridge protocol, emqx_bridge_proto_v4, in the emqx_bridge app. The new protocol improves the efficiency and scalability of bridge metrics retrieval and separates them from the bridge status and configuration data. The pull request also updates the emqx_authn and emqx_authz apps to use the new protocol and modifies some test cases accordingly.

PR Checklist

Please convert it to a draft if any of the following conditions are not met. Reviewers may skip over until all the items are checked:

Added tests for the changes
Changed lines covered in coverage report
Change log has been added to changes/{ce,ee}/(feat|perf|fix)-<PR-id>.en.md files
~~For internal contributor: there is a jira ticket to track this change~~
~~If there should be document changes, a PR to emqx-docs.git is sent, or a jira ticket is created to follow up~~
Schema changes are backward compatible

coveralls · 2023-04-10T15:54:29Z

Pull Request Test Coverage Report for Build 4665751511

35 of 38 (92.11%) changed or added relevant lines in 8 files are covered.
53 unchanged lines in 16 files lost coverage.
Overall coverage decreased (-0.06%) to 81.019%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
apps/emqx_bridge/src/emqx_bridge_api.erl	19	20	95.0%
apps/emqx_bridge/src/proto/emqx_bridge_proto_v3.erl	0	1	0.0%
apps/emqx_bridge/src/proto/emqx_bridge_proto_v4.erl	9	10	90.0%

Files with Coverage Reduction	New Missed Lines	%
apps/emqx_bridge/src/emqx_bridge.erl	1	91.27%
apps/emqx_mqttsn/src/emqx_mqttsn_frame.erl	1	63.48%
apps/emqx_rule_engine/src/emqx_rule_engine.erl	1	86.23%
apps/emqx/src/emqx_broker_helper.erl	1	90.7%
apps/emqx/src/emqx_limiter/src/emqx_limiter_server.erl	1	89.08%
apps/emqx/src/emqx_session.erl	1	87.25%
apps/emqx/src/emqx_stats.erl	1	93.33%
apps/emqx_dashboard/src/emqx_dashboard_monitor.erl	2	63.89%
apps/emqx/src/emqx_broker.erl	2	87.64%
apps/emqx/src/emqx_channel.erl	2	87.48%

Totals
Change from base Build 4642703850:	-0.06%
Covered Lines:	25103
Relevant Lines:	30984

💛 - Coveralls

changes/ce/feat-10359.en.md

savonarola · 2023-04-11T13:25:07Z

apps/emqx_bridge/src/emqx_bridge_api.erl

@@ -990,7 +988,7 @@ do_bpapi_call(Node, Call, Args) ->
 do_bpapi_call_vsn(SupportedVersion, Call, Args) ->
    case lists:member(SupportedVersion, supported_versions(Call)) of
        true ->
-            apply(emqx_bridge_proto_v3, Call, Args);
+            apply(emqx_bridge_proto_v4, Call, Args);


Nit: isn't this a kind of evading from some of the bpapi checks? 🤔

What scenario do you have in mind? I believe that piece of logic always (silently) assumed that the type (i.e. signature) of an existing RPC does not change across protocol versions. Which in general is not true, but I'm not aware of bpapi checks for that.

I mean, dialyzer won't check that Args type matches the bpapi's wrapper type; only that bpapi's wrapper declared type matches the type of the corresponding remote call.

Co-Authored-By: ieQu1 <99872536+ieQu1@users.noreply.github.com>

thalesmg · 2023-04-11T13:29:27Z

apps/emqx_resource/src/emqx_resource_manager.erl

@@ -37,7 +37,6 @@
    list_all/0,


Just curiosity: list_all/0 still collects the metrics, and it seems to be used only by emqx_resource:list_instances_verbose, which in turn seems to be for debugging purposes. Maybe we can remove data_record_to_external_map_with_metrics?

I'm all for it. Just to be clear: do you mean drop metrics from list_instances_verbose/0 output altogether? Or keep the behavior, change the place where metrics are collected?

I think the latter would be better, so we could drop the ..._map_with_metrics fn and use the same API to collect metrics when calling this debug function. What do you think?

I think mostly the same, did that in 9c9f39d.

savonarola · 2023-04-11T13:41:07Z

apps/emqx_bridge/src/emqx_bridge_api.erl

@@ -990,7 +988,7 @@ do_bpapi_call(Node, Call, Args) ->
 do_bpapi_call_vsn(SupportedVersion, Call, Args) ->
    case lists:member(SupportedVersion, supported_versions(Call)) of
        true ->
-            apply(emqx_bridge_proto_v3, Call, Args);
+            apply(emqx_bridge_proto_v4, Call, Args);


I mean, dialyzer won't check that Args type matches the bpapi's wrapper type; only that bpapi's wrapper declared type matches the type of the corresponding remote call.

savonarola · 2023-04-11T13:43:34Z

Things got much more readable when they got separated.

Do I understand correctly that cluster calls in bridge API ware needed only for metrics?

keynslug · 2023-04-11T13:52:34Z

Do I understand correctly that cluster calls in bridge API ware needed only for metrics?

I wouldn't say so, pretty much all RPCs in emqx_bridge_proto_v4 are in use now (as shown by coverage report). Previously a single RPC (lookup_from_all_nodes) was used for both bridge listing and metrics API operations. Now they are two different RPCs, so simple emqx_resource:get_instance/1 is unencumbered by metrics collection.

Now `emqx_resource:list_instances_verbose/0` will populate the metrics for each instance, for the sake of simplicity.

keynslug requested review from a team, JimMoen, savonarola and lafirest as code owners April 10, 2023 15:04

feat(resource): ask for metrics only when needed

e70deae

keynslug force-pushed the ft/EMQX-9136/no-ask-metrics branch from a71ce2e to e70deae Compare April 11, 2023 09:01

ieQu1 reviewed Apr 11, 2023

View reviewed changes

changes/ce/feat-10359.en.md Outdated Show resolved Hide resolved

savonarola reviewed Apr 11, 2023

View reviewed changes

chore: add changelog entry

d5ae5eb

Co-Authored-By: ieQu1 <99872536+ieQu1@users.noreply.github.com>

keynslug force-pushed the ft/EMQX-9136/no-ask-metrics branch from 2b6e4cd to d5ae5eb Compare April 11, 2023 13:27

thalesmg previously approved these changes Apr 11, 2023

View reviewed changes

savonarola previously approved these changes Apr 11, 2023

View reviewed changes

keynslug dismissed stale reviews from savonarola and thalesmg via 72bcf7c April 12, 2023 13:12

feat(resman): also move out metrics collection for debugging

9c9f39d

Now `emqx_resource:list_instances_verbose/0` will populate the metrics for each instance, for the sake of simplicity.

keynslug force-pushed the ft/EMQX-9136/no-ask-metrics branch from 72bcf7c to 9c9f39d Compare April 12, 2023 13:14

keynslug requested a review from thalesmg April 13, 2023 07:14

savonarola approved these changes Apr 14, 2023

View reviewed changes

keynslug merged commit 5e92ba6 into emqx:master Apr 14, 2023
108 checks passed

keynslug deleted the ft/EMQX-9136/no-ask-metrics branch April 14, 2023 09:28

p-linnane mentioned this pull request Apr 18, 2023

emqx 5.0.23 Homebrew/homebrew-core#128719

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(resource): ask for metrics only when needed #10359

feat(resource): ask for metrics only when needed #10359

keynslug commented Apr 10, 2023 •

edited

coveralls commented Apr 10, 2023 •

edited

savonarola Apr 11, 2023

keynslug Apr 11, 2023 •

edited

savonarola Apr 11, 2023

thalesmg Apr 11, 2023

keynslug Apr 11, 2023

thalesmg Apr 12, 2023

keynslug Apr 12, 2023

savonarola Apr 11, 2023

savonarola commented Apr 11, 2023

keynslug commented Apr 11, 2023

feat(resource): ask for metrics only when needed #10359

feat(resource): ask for metrics only when needed #10359

Conversation

keynslug commented Apr 10, 2023 • edited

Summary

🤖 Generated by Copilot at a71ce2e

PR Checklist

coveralls commented Apr 10, 2023 • edited

Pull Request Test Coverage Report for Build 4665751511

💛 - Coveralls

savonarola Apr 11, 2023

Choose a reason for hiding this comment

keynslug Apr 11, 2023 • edited

Choose a reason for hiding this comment

savonarola Apr 11, 2023

Choose a reason for hiding this comment

thalesmg Apr 11, 2023

Choose a reason for hiding this comment

keynslug Apr 11, 2023

Choose a reason for hiding this comment

thalesmg Apr 12, 2023

Choose a reason for hiding this comment

keynslug Apr 12, 2023

Choose a reason for hiding this comment

savonarola Apr 11, 2023

Choose a reason for hiding this comment

savonarola commented Apr 11, 2023

keynslug commented Apr 11, 2023

keynslug commented Apr 10, 2023 •

edited

`🤖 Generated by Copilot at a71ce2e`

coveralls commented Apr 10, 2023 •

edited

keynslug Apr 11, 2023 •

edited