Fix #158: Add non-UI telemetry probes #240

biancadanforth · 2018-11-01T18:43:54Z

Broke this up into a few commits. Also updated the extension's name and telemetry category in METRICS.md and added a new Appendix section to explain how to find the probes (mostly for QA).

I don’t like how I get the “previous price” (I refetch state) for the detect_price_change probe, but I didn't see another way.

biancadanforth · 2018-11-01T18:44:37Z

@Osmose , ready for your review!

Osmose

Nice work, I'm glad we were able to get to this in the end!

Osmose · 2018-11-01T20:55:13Z

src/background/price_alerts.js

@@ -86,10 +95,12 @@ export async function handleNotificationClicked(notificationId) {
    // Record open_external_page event
    const latestPrice = getLatestPriceForProduct(state, product.id);
    const originalPrice = getOldestPriceForProduct(state, product.id);
+    const highPriceAmount = Dinero({amount: alert.highPriceAmount});


No need to convert this; if we're not comparing it to other amounts or displaying it, the Dinero object adds nothing (and honestly it's adding almost nothing right now anyway, I kinda regret including it before supporting multiple currencies).

Osmose · 2018-11-01T21:12:12Z

src/background/price_updates.js

  const state = store.getState();
  const id = getProductIdFromExtracted(data);
  const product = getProduct(state, id);
  if (product) {
-    store.dispatch(addPriceFromExtracted(data));
+    const isPriceChange = await store.dispatch(addPriceFromExtracted(data));


This is only run when there's an existing product, which means we already have at least one price logged. This method has an if check inside that checks whether the price being added is the first price logged, but given that we only care about the return value here, and we already know that the if statement will be true, we can skip it completely.

Instead, what if addPriceFromExtracted returns either the price that was added or null? That way, we don't need to fetch the latest price below at all. If the only prices we need to fetch are the previous price and original price, we can fetch those using the previous state object and avoid calling getState again.

Osmose · 2018-11-01T21:16:40Z

src/extraction/index.js

-function extractProduct() {
-  for (const extract of EXTRACTION_METHODS) {
+function extractProduct(isBackgroundUpdate) {
+  const baseExtra = {


The amount of events in this method are making it difficult to follow, what if we had a more concise API?

function extractProduct() { const attempt = new ExtractionAttempt(); attempt.start(); for (const extract of EXTRACTION_METHODS) { const extractedProduct = extract(window.document); if (extractedProduct) { attempt.succeed(methodName); return extractedProduct; } } attempt.fail(); return null; }

That's a great idea; thank you!

Osmose · 2018-11-01T21:19:09Z

src/background/extraction.js

@@ -51,6 +52,11 @@ export async function handleExtractedProductData({extractedProduct}, sender) {
      tabId,
    });
    browser.browserAction.setBadgeText({text: '✚', tabId});
+    if (sendTelemetry) {


What's the purpose of this? To prevent sending events when the badge isn't changing from its previous value?

If so, couldn't we just compare the old and new values instead of passing a parameter around?

I added that second argument to sendProductToBackground because when we call resend (the messy workaround for Bug 1493470 in PR #220 ), we are also re-recording the badge_toolbar_button event (for badge type of 'add'). It is a temporary addition to the temporary code to also be removed when resend is removed.

Although your idea of only recording the event (for badge types of 'add' and 'price_alert') if the previous badge value is different would also work here. I'll do that instead, since that can stay even after resend is removed.

Geeze this was a lot trickier than I anticipated. More soon when I resubmit the PR...

I am still seeing duplicate detect_price_change events due to resend:

[STR]:

Open the fake product page in Tab 1, say it has Price $A.

Add the product via the browserAction toolbar button

Refresh the page until a Price Alert is triggered, Price $B.

Immediately click on the System Notification (within 5 seconds of refreshing the page that triggered the alert)

This will open another fake product page in Tab 2 (at Price $C) and perform extraction before resend has been executed even once in Tab 1 fake product page.

Because the price is different between Tabs 1 and 2, the price history will look something like: [A, ... (refreshing to trigger a change), B, C, B (resend), C (resend), B (resend), C (resend)], where all the resend prices are duplicate events which can incorrectly trigger price alerts.

I don't think this will be a problem in any real world scenario, since it would require a product page's price to change within 5 seconds of visiting the page.

@Osmose , do you agree that this is a non-issue?

Yeah, this should be fine.

Osmose · 2018-11-01T21:21:57Z

src/extraction/index.js

-async function attemptExtraction() {
-  const extractedProduct = extractProduct();
+async function attemptExtraction(isBackgroundUpdate) {
+  const extractedProduct = extractProduct(isBackgroundUpdate);


Instead of passing this parameter through a bunch of methods, if we move the isBackgroundUpdate code into a function, we can call it again when we need to log the value a second time. We could also assign it to a global variable, as whether the page is a background update or not is not expected to change within this script.

biancadanforth · 2018-11-03T00:01:55Z

@Osmose Ready for another look!

@teonbrooks I haven't heard back from you re: collapsing the privacy extra keys into a single key; if there's still no feedback by Monday (11/5/18), I will proceed with that approach in this PR.

teonbrooks · 2018-11-03T13:24:35Z

sorry for the delay. for the extra keys, I would go with the splitting, if possible, that follow the event telemetry schema. it would make for straightforward analysis

Osmose

Thank you, nice work!

Osmose · 2018-11-05T22:41:23Z

src/telemetry/extension.js

+export async function hasBadgeTextChanged(nextText, tabId = null) {
+  // If the nextText contains a digit, we know it's a price alert badge, which only affects
+  // the global badge text.
+  const returnGlobal = (/\d+/.test(nextText));


Wouldn't tabId being null be a better signal that we want to check the global badge text?

Osmose · 2018-11-05T22:41:52Z

src/telemetry/extension.js

+  if (prevText !== nextText) {
+    return true;
+  }
+  return false;


We can replace this conditional with:

return prevText !== nextText;

Osmose · 2018-11-05T22:44:18Z

src/telemetry/extension.js

@@ -229,6 +231,19 @@ export async function getBadgeType() {
  }
 }

+export async function hasBadgeTextChanged(nextText, tabId = null) {


This API is difficult to read at the point of usage; hasBadgeTextChanged('+') doesn't really make it clear that the parameter is the badge text we're intending to change to.

Since it's being passed in anyway, maybe change this to getToolbarBadgeText(tabId) and do the comparison in place?

if (getToolbarBadgeText(tabId) !== '+') { }

Osmose · 2018-11-05T22:52:05Z

src/extraction/index.js

@@ -28,40 +28,72 @@ const EXTRACTION_METHODS = {
  open_graph: extractProductWithOpenGraph,
 };

+const IS_BACKGROUND_UPDATE = (function isBackgroundUpdate() {


Fun fact, since we use let and const now we don't need to use functions to hide one-time computations, blocks work just as well:

let isBackgroundUpdate = false; { // This is a block! They are neat! let result = false; try { isBackgroundUpdate = window.top.location.href.startsWith( browser.runtime.getURL('/'), // URL is unique per-install / hard to forge ); } catch (err) { // Non-background updates may throw a cross-origin error } }

although since we don't need the intermediate result value anyway we can simplify even more:

let isBackgroundUpdate = false; try { isBackgroundUpdate = window.top.location.href.startsWith( browser.runtime.getURL('/'), // URL is unique per-install / hard to forge ); } catch (err) { // Non-background updates may throw a cross-origin error }

Yeah I haven't really made use of blocks, but I know what you mean in that const and let are block-scoped, and I don't need a function in this case. Makes sense.

Osmose · 2018-11-05T22:55:19Z

src/extraction/index.js

+/**
+ * Helper class to record extraction-related telemetry events.
+ */
+class ExtractionAttempt {


That guy looks pretty smug. He wishes he had Osmose's antlers!

biancadanforth · 2018-11-06T05:35:30Z

@Osmose , thanks for the feedback. This is ready for another look!

Osmose

Looks great, thank you for this!

Osmose · 2018-11-06T19:05:09Z

src/telemetry/extension.js

+  // The 'price_alert' badge modifies the global badge text and will not have a tabId.
+  const returnGlobal = !tabId;
+  return browser.browserAction.getBadgeText(
+    returnGlobal ? {} : {tabId},


We may optionally consider dropping the variable:

return browser.browserAction.getBadgeText(tabId ? {tabId} : {});

Add 'detect_price_change' probe * Add `price_prev` extra key for `detect_price_change` event. This will be the same value as `price_orig` if there is only one price entry for the product when updating prices. This will help us answer questions around how much prices change in addition to how frequently. Additionally: * Update the name of the extension in METRICS.md to Price Wise. * Add Appendix B to METRICS.md which explains how to view recorded telemetry events in about:telemetry.

Add badge_toolbar_button and send_notification probes: * Add an additional extra key, 'price_last_high' to 'send_notification' and 'open_external_page' (with 'element' value of 'system_notification') and update METRICS.md. * Add a 'sendTelemetry' flag for the 'sendProductToBackground' function to also be removed when the 'resend' code is removed in './src/extraction/index.js'. * Due to the fix for Issue mozilla#192, I was seeing some extra 'badge_toolbar_button' events (with 'badge_type' of 'add') as we are resending extracted product information 5 and 10 seconds after the product page loads. * Update METRICS.md to clarify when 'badge_toolbar_button' with a 'badge_type' of 'price_alert' fires, since it will continue to fire for the same price alert until that alert is dismissed which is not obvious.

…robes * Update 'method' extra key for 'complete_extraction' event to have values of 'fathom', 'css_selectors', 'open_graph' or 'none' to distinguish between the two fallback extraction methods to Fathom: CSS Selectors or Open Graph attributes. * Note: Since none of our five supported sites use Open Graph attributes currently for product information, we should not expect to see any successful extraction using that method for the MVP. * Important caveats for making conclusions using these probes as-is: * The coverage values that these probes will suggest will not be very accurate initially until we ensure we are extracting on only **product pages** for a supported site, rather than any page on the site (see Issues mozilla#225 and mozilla#181). * Successful extraction does not mean that the information extracted for the product is correct. It only means that _a_ value was extracted for each product feature (i.e. title, image and price) on the page.

Event Telemetry [limits the max number of extra keys](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/events.html#limits) for an event to 10. In the fix for Issue mozilla#185, where three new privacy-related extra keys were added to every telemetry event, the number of extra keys for the 'open_external_page' event went to 11, which causes event telemetry to fail at the registration step. This patch collapses these 3 privacy keys into a single key that points to a stringified JSON object.

Break up Extra Keys section into Common Extra Keys and Event-specific Extra Keys subsections. Update Sample Ping section to include additional common keys added from Issue mozilla#185.

Issue mozilla#157's patch removed this event from METRICS.md, but failed to remove it from the code. The uninstall event is recorded by the Addons Manager. It is not possible to record from a WE, as the extension is uninstalled before the event can be recorded.

* Note: This fix is complicated by the fact that a 'price alert' badge only changes the global badge state. An 'add' product badge changes the tab-specific badge state, which takes precedence over the global badge state. To avoid event duplication for 'badge_toolbar_button': * Add 'hasBadgeTextChanged' function with optional 'tabId' argument, so that the event only fires if the badge text has changed. * Previously, we were getting a bunch of redundant `badge_toolbar_button` events for ‘price_alert’ any time the state was changed and we’re on a page with an ‘add’ badge. Since `handlePriceAlerts` is called every time state changes in any way (`store.subscribe(handlePriceAlerts)`), we now check to see if the badge text has changed for a price alert badge on the global browserAction button before recording the 'badge_toolbar_button' with badge type 'price_alert' event. * Previously, if tab 1 had badge type 'add' and tab 2 had badge type 'price_alert', and I switch to tab 2 before any calls to 'resend' (mozilla#220) are made from tab 1, I would get two extra 'badge_toolbar_button' of type 'add' events. Adding the 'tabId' optional argument fixes this bug.

… events This patch: * Reverts "Collapse privacy extra keys into a single key" (commit 32fcfc4) while preserving the Common Extra Keys and Event-Specific Extra Keys subsections and updating the Sample Ping section in the METRICS.md document. * Breaks out 'open_external_page' event (11 extra keys) into two events: 'open_nonproduct_page' (5 extra keys) and 'open_product_page' (10 extra keys).

biancadanforth requested a review from Osmose November 1, 2018 18:43

Osmose suggested changes Nov 1, 2018

View reviewed changes

biancadanforth force-pushed the 158-non-ui-probes branch from b7354bb to abec33d Compare November 2, 2018 23:54

biancadanforth requested review from Osmose and teonbrooks November 2, 2018 23:55

biancadanforth added the [ENG]: Do not merge Do not merge this pull request label Nov 3, 2018

biancadanforth removed the [ENG]: Do not merge Do not merge this pull request label Nov 3, 2018

biancadanforth removed the request for review from teonbrooks November 3, 2018 20:58

Osmose suggested changes Nov 5, 2018

View reviewed changes

biancadanforth force-pushed the 158-non-ui-probes branch from 3a4c3af to 1db8a9e Compare November 6, 2018 05:33

biancadanforth requested a review from Osmose November 6, 2018 05:34

Osmose approved these changes Nov 6, 2018

View reviewed changes

biancadanforth added 11 commits November 6, 2018 13:04

Rename telemetry event category to 'extension.price_wise'

6bf7151

Update METRICS.md

e633803

Break up Extra Keys section into Common Extra Keys and Event-specific Extra Keys subsections. Update Sample Ping section to include additional common keys added from Issue mozilla#185.

Incorporate more of Osmose's feedback

44391b7

Remove unnecessary 'returnGlobal' variable

96b7a24

biancadanforth force-pushed the 158-non-ui-probes branch from 1db8a9e to 96b7a24 Compare November 6, 2018 21:09

biancadanforth merged commit 538268a into mozilla:master Nov 6, 2018

biancadanforth deleted the 158-non-ui-probes branch November 6, 2018 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #158: Add non-UI telemetry probes #240

Fix #158: Add non-UI telemetry probes #240

biancadanforth commented Nov 1, 2018 •

edited

Loading

biancadanforth commented Nov 1, 2018

Osmose left a comment

Osmose Nov 1, 2018

Osmose Nov 1, 2018

Osmose Nov 1, 2018

biancadanforth Nov 2, 2018

Osmose Nov 1, 2018

biancadanforth Nov 2, 2018 •

edited

Loading

biancadanforth Nov 2, 2018

biancadanforth Nov 3, 2018

Osmose Nov 5, 2018

Osmose Nov 1, 2018

biancadanforth commented Nov 3, 2018

teonbrooks commented Nov 3, 2018

Osmose left a comment

Osmose Nov 5, 2018

Osmose Nov 5, 2018

Osmose Nov 5, 2018

Osmose Nov 5, 2018

biancadanforth Nov 6, 2018 •

edited

Loading

Osmose Nov 5, 2018

chuckharmston Nov 5, 2018

biancadanforth Nov 6, 2018 •

edited

Loading

biancadanforth commented Nov 6, 2018

Osmose left a comment

Osmose Nov 6, 2018

Fix #158: Add non-UI telemetry probes #240

Fix #158: Add non-UI telemetry probes #240

Conversation

biancadanforth commented Nov 1, 2018 • edited Loading

biancadanforth commented Nov 1, 2018

Osmose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth commented Nov 3, 2018

teonbrooks commented Nov 3, 2018

Osmose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

biancadanforth commented Nov 6, 2018

Osmose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth commented Nov 1, 2018 •

edited

Loading

biancadanforth Nov 2, 2018 •

edited

Loading

biancadanforth Nov 6, 2018 •

edited

Loading

biancadanforth Nov 6, 2018 •

edited

Loading