Include number of total and blocked trackers in Telemetry (fixes #29) #61
Conversation
ericawright
left a comment
ericawright
left a comment
There was a problem hiding this comment.
It doesn't seem to actually send telemetry anymore, and the test for that is failing.
|
Which test is failing for you? The one in |
|
1-telemetry.js |
src/privileged/trackers/api.js
Outdated
| emitTrackersExist(tabId) { | ||
| this.emit("trackers-exist", {tabId}); | ||
| emitTrackersExist(tabId, trackersFound, trackersBlocked) { | ||
| this.emit("trackers-exist", {tabId, trackersFound, trackersBlocked}); |
There was a problem hiding this comment.
Do you want to unwrap these?
| const isAllowing = state & Ci.nsIWebProgressListener.STATE_LOADED_TRACKING_CONTENT; | ||
| if (isBlocking || isAllowing) { | ||
| // There are trackers on this page. | ||
| // This information should only be stored on the top-level docshell, so we use that even in frames. |
There was a problem hiding this comment.
does this mean any trackers in an iframe are missed, or they will just be saved on the top level?
There was a problem hiding this comment.
Assuming this was implemented as specified (which it looks like) then this will just save all blocked trackers (even from sub-frames) on the top level document.
| driver = await utils.setupWebdriver.promiseSetupDriver( | ||
| utils.FIREFOX_PREFERENCES, | ||
| ); | ||
| await utils.setPreference(driver, "extensions.fastblock-shield_mozilla_org.test.variationName", "TPL0"); |
There was a problem hiding this comment.
I don't like that this has to be set, I'm getting failures. If I set it to "FB5L0" there's a failure on "correctly records performance metrics"
on "Control" there's a failure on "has recorded one ping"
Doesn't this sort of invalidate the tests?
Since this branch is needed in order to record amount of trackers on a page, couldn't we just set this before running that test in particular.
There was a problem hiding this comment.
shoot, I think the errors are not your fault, I'm seeing them on master too - but only with certain branches so it's unpredictable. Sometimes I get a positive test, other failing
There was a problem hiding this comment.
Yeah for some reason https://mozilla.github.io/tracking-test/disconnect.html doesn't always get blocked in this test but seems to work fine in real life. It's a little worrying, but I suppose nothing that needs to concern us for this PR :(
There was a problem hiding this comment.
I replaced it with our well-working default test site: https://itisatrap.org/firefox/its-a-tracker.html
|
So we're currently resolving the mentioned issues in https://bugzilla.mozilla.org/show_bug.cgi?id=1485400. As I wrote in that bug, the patch seems to get the right behavior when testing with framescripts but that didn't seem to translate to the Shield study in my testing. In any case, I used a debug build and that was pretty messed up and crashy with the study anyway, so I'd like to wait and see how this behaves when we have it in Nightly. I'm going to fix the conflicts and nits and land this. |
This takes the numbers from the document, although there are a few flaws in the implementation on Necko side, which I will raise with them. See tests that are currently skipped. Basically this feature only works well if Tracking Protection is turned on.
|
@ericawright do you think this is good to merge? |
This takes the numbers from the document, although there are a few
flaws in the implementation on Necko side, which I will raise with
them. See tests that are currently skipped.
Basically this feature only works well if Tracking Protection is turned
on.