From f21ff3f485ee144d0c6bb2010d59761204f9dc56 Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Wed, 5 Apr 2023 10:36:12 +0000 Subject: [PATCH] Bug 1803363 - Record metrics related to the amount of DNR rules enabled during DNR rule evaluation. r=robwu Differential Revision: https://phabricator.services.mozilla.com/D174026 --- .../extensions/ExtensionDNR.sys.mjs | 26 +++++++ toolkit/components/extensions/metrics.yaml | 16 +++++ .../xpcshell/test_ext_dnr_static_rules.js | 67 +++++++++++++++---- 3 files changed, 96 insertions(+), 13 deletions(-) diff --git a/toolkit/components/extensions/ExtensionDNR.sys.mjs b/toolkit/components/extensions/ExtensionDNR.sys.mjs index 616ba51bb421..173c803d10d8 100644 --- a/toolkit/components/extensions/ExtensionDNR.sys.mjs +++ b/toolkit/components/extensions/ExtensionDNR.sys.mjs @@ -1808,6 +1808,8 @@ class RequestEvaluator { } const NetworkIntegration = { + maxEvaluatedRulesCount: 0, + register() { // We register via WebRequest.jsm to ensure predictable ordering of DNR and // WebRequest behavior. @@ -1852,6 +1854,14 @@ const NetworkIntegration = { ); } } + const evaluateRulesCount = ruleManagers.reduce( + (sum, ruleManager) => sum + ruleManager.getRulesCount(), + 0 + ); + if (evaluateRulesCount > this.maxEvaluatedRulesCount) { + Glean.extensionsApisDnr.evaluateRulesCountMax.set(evaluateRulesCount); + this.maxEvaluatedRulesCount = evaluateRulesCount; + } } // Cache for later. In case of redirects, _dnrMatchedRules may exist for // the pre-redirect HTTP channel, and is overwritten here again. @@ -1980,6 +1990,7 @@ class RuleManager { this.hasBlockPermission = extension.hasPermission("declarativeNetRequest"); this.hasRulesWithTabIds = false; this.hasRulesWithAllowAllRequests = false; + this.totalRulesCount = 0; } get availableStaticRuleCount() { @@ -2002,7 +2013,10 @@ class RuleManager { } setSessionRules(validatedSessionRules) { + let oldRulesCount = this.sessionRules.rules.length; + let newRulesCount = validatedSessionRules.length; this.sessionRules.rules = validatedSessionRules; + this.totalRulesCount += newRulesCount - oldRulesCount; this.hasRulesWithTabIds = !!this.sessionRules.rules.find(rule => { return rule.condition.tabIds || rule.condition.excludedTabIds; }); @@ -2011,7 +2025,10 @@ class RuleManager { } setDynamicRules(validatedDynamicRules) { + let oldRulesCount = this.dynamicRules.rules.length; + let newRulesCount = validatedDynamicRules.length; this.dynamicRules.rules = validatedDynamicRules; + this.totalRulesCount += newRulesCount - oldRulesCount; this.#updateAllowAllRequestRules(); } @@ -2030,7 +2047,12 @@ class RuleManager { this.makeRuleset(id, idx + PRECEDENCE_STATIC_RULESETS_BASE, rules) ); } + const countRules = rulesets => + rulesets.reduce((sum, ruleset) => sum + ruleset.rules.length, 0); + const oldRulesCount = countRules(this.enabledStaticRules); + const newRulesCount = countRules(rulesets); this.enabledStaticRules = rulesets; + this.totalRulesCount += newRulesCount - oldRulesCount; this.#updateAllowAllRequestRules(); } @@ -2042,6 +2064,10 @@ class RuleManager { return this.dynamicRules.rules; } + getRulesCount() { + return this.totalRulesCount; + } + #updateAllowAllRequestRules() { const filterAAR = rule => rule.action.type === "allowAllRequests"; this.hasRulesWithAllowAllRequests = diff --git a/toolkit/components/extensions/metrics.yaml b/toolkit/components/extensions/metrics.yaml index 3d9c8c99d4f4..a93100d8cb6a 100644 --- a/toolkit/components/extensions/metrics.yaml +++ b/toolkit/components/extensions/metrics.yaml @@ -146,3 +146,19 @@ extensions.apis.dnr: - https://bugzilla.mozilla.org/show_bug.cgi?id=1803363#c11 data_sensitivity: - technical + + evaluate_rules_count_max: + type: quantity + unit: rules + expires: 120 + description: | + Max amount of DNR rules being evaluated. + lifetime: ping + notification_emails: + - addons-dev-internal@mozilla.com + bugs: + - https://bugzilla.mozilla.org/1803363/ + data_reviews: + - https://bugzilla.mozilla.org/show_bug.cgi?id=1803363#c11 + data_sensitivity: + - technical diff --git a/toolkit/components/extensions/test/xpcshell/test_ext_dnr_static_rules.js b/toolkit/components/extensions/test/xpcshell/test_ext_dnr_static_rules.js index b25e84471049..a94b000d78c8 100644 --- a/toolkit/components/extensions/test/xpcshell/test_ext_dnr_static_rules.js +++ b/toolkit/components/extensions/test/xpcshell/test_ext_dnr_static_rules.js @@ -1627,12 +1627,13 @@ add_task(async function test_static_rules_telemetry() { info("Verify telemetry recorded on rules evaluation"); extension.sendMessage("updateEnabledRulesets", { enableRulesetIds: ["ruleset1"], + disableRulesetIds: ["ruleset2"], }); await extension.awaitMessage("updateEnabledRulesets:done"); - await assertDNRGetEnabledRulesets(extension, ["ruleset1", "ruleset2"]); + await assertDNRGetEnabledRulesets(extension, ["ruleset1"]); assertDNRTelemetryMetricsNoSamples( - ["evaluateRulesTime"], + ["evaluateRulesTime", "evaluateRulesCountMax"], "before any request have been intercepted" ); @@ -1643,11 +1644,23 @@ add_task(async function test_static_rules_telemetry() { ); assertDNRTelemetryMetricsNoSamples( - ["evaluateRulesTime"], + ["evaluateRulesTime", "evaluateRulesCountMax"], "after restricted request have been intercepted (but no rules evaluated)" ); const page = await ExtensionTestUtils.loadContentPage("http://example.com"); + const callPageFetch = async () => { + Assert.equal( + await page.spawn([], () => { + return this.content.fetch("http://example.com/").then( + res => res.text(), + err => err.message + ); + }), + "NetworkError when attempting to fetch resource.", + "DNR should have blocked test request to example.com" + ); + }; // Expect one sample recorded on evaluating rules for the // top level navigation. @@ -1657,18 +1670,16 @@ add_task(async function test_static_rules_telemetry() { expectedEvaluateRulesTimeSamples, "evaluateRulesTime should be collected after evaluated rulesets" ); - - Assert.equal( - await page.spawn([], () => { - return this.content.fetch("http://example.com/").then( - res => res.text(), - err => err.message - ); - }), - "NetworkError when attempting to fetch resource.", - "DNR should have blocked test request to example.com" + // Expect 1 rule with only one ruleset enabled. + let expectedEvaluateRulesCountMax = 1; + assertDNRTelemetryMetricsGetValueEq( + ["evaluateRulesCountMax"], + expectedEvaluateRulesCountMax, + "evaluateRulesCountMax should be collected after evaluated rulesets" ); + await callPageFetch(); + // Expect one new sample reported on evaluating rules for the // top level navigation. expectedEvaluateRulesTimeSamples += 1; @@ -1678,6 +1689,36 @@ add_task(async function test_static_rules_telemetry() { "evaluateRulesTime should be collected after evaluated rulesets" ); + extension.sendMessage("updateEnabledRulesets", { + enableRulesetIds: ["ruleset2"], + }); + await extension.awaitMessage("updateEnabledRulesets:done"); + await assertDNRGetEnabledRulesets(extension, ["ruleset1", "ruleset2"]); + + await callPageFetch(); + + // Expect two rules with both rulesets enabled. + expectedEvaluateRulesCountMax += 1; + assertDNRTelemetryMetricsGetValueEq( + ["evaluateRulesCountMax"], + expectedEvaluateRulesCountMax, + "evaluateRulesCountMax should have been increased" + ); + + extension.sendMessage("updateEnabledRulesets", { + disableRulesetIds: ["ruleset2"], + }); + await extension.awaitMessage("updateEnabledRulesets:done"); + await assertDNRGetEnabledRulesets(extension, ["ruleset1"]); + + await callPageFetch(); + + assertDNRTelemetryMetricsGetValueEq( + ["evaluateRulesCountMax"], + expectedEvaluateRulesCountMax, + "evaluateRulesCountMax should have not been decreased" + ); + await page.close(); await extension.unload();