Closed Bug 1100914 Opened 5 years ago Closed 5 years ago

Switch context menu telemetry to a serialized array of states instead of a single string

Categories

(Firefox :: Menus, defect)

34 Branch
defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
37.1

People

(Reporter: VarCat, Assigned: Gijs)

Details

Attachments

(1 file)

No description provided.
FF 34
Build Id:20141117202603

STR:
1. Start the Firefox promotional video from about:home page.
2. On about:home page select some of the characters from the page.
3. Right click on the video.
4. Press Show Statistics.

Issue:
A context menu is opened that displays option specifics to both media and selection.
The interaction with the context menu even if it's related to media it will be recorded under selection on about:telemetry simple measurements.
Thanks, Catalin!

So, the state of the context menu here is, IMO, correct. There is selected text, and if you then right click, we always display the selection part of the menu (also so you can then right click elsewhere and still have it work, AIUI, and equally I'm not sure how trivial it is to figure out if you did right click within the selection or not, especially if the selection is part of the same text node as the one you clicked). Equally, it would be unexpected if we dropped the video part of the menu if you right-clicked the video... Jared, is that right?

Assuming I'm right about the menu itself, what do we want for the telemetry here? It's going to be wrong whatever we end up picking. We could make the "type" string we currently pass concatenate all the states that apply instead, I guess? Would that be less weird? Blake?
Flags: needinfo?(jaws)
Flags: needinfo?(bwinton)
Instead of a concatenation, I think an array of applicable type-strings would make more sense…  (If possible.)
Flags: needinfo?(bwinton)
(In reply to Blake Winton (:bwinton) from comment #4)
> Instead of a concatenation, I think an array of applicable type-strings
> would make more sense…  (If possible.)

These are object keys in the JSON that's sent. You can't use arrays as regular JS object keys - those are always strings.
Could I sell you on a JSON-stringify-ed copy of the array, then?  (If not, we can parse whatever format back into an array when we do the analysis, but having it be array-like would simplify things on the back end.  ;)
(In reply to Blake Winton (:bwinton) from comment #6)
> Could I sell you on a JSON-stringify-ed copy of the array, then?  (If not,
> we can parse whatever format back into an array when we do the analysis, but
> having it be array-like would simplify things on the back end.  ;)

ugly, but it would work, I guess. :-(

I can see what I can do over the rest of this week (but I keep telling people that, so you may need to wait longer). Also, how do we deal with the changing format? Is that not going to be an issue? :-)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bwinton)
I guess taking for accountability purposes, but I am wary of trying to make this make 34... is that OK?
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Points: --- → 3
Flags: qe-verify+
Flags: needinfo?(gijskruitbosch+bugs)
Flags: firefox-backlog+
Summary: Media context menu interaction recorded as selection context menu interaction. → Switch context menu telemetry to a serialized array of states instead of a single string
We could check for a leading "["…  I suppose if we switched to a more backwards-compatible format, then we could handle older versions as well as newer versions, but I think having the format be obviously different will help us tell whether this is a click on just a selection (new-style), or on a selection possibly with other stuff (old-style)…

It would be nice to get it in, but we're all quite busy, so if it doesn't make it, I'm not going to complain.  Well, not too loudly…  ;)

(Thanks for working on it!)
Flags: needinfo?(bwinton)
Assignee: nobody → gijskruitbosch+bugs
This should do the trick, I think? Blake, can you check that you can work with this data? I expect it will take some work to tease out what you want considering that now e.g. hits for 'video' will be spread out between all the things that have video as part of the bools that were true at the point where the user context-clicked...
Attachment #8526069 - Flags: review?(jaws)
Attachment #8526069 - Flags: feedback?(bwinton)
Comment on attachment 8526069 [details] [diff] [review]
switch telemetry to use an array of strings to indicate all states,

This seems good to me, and I'm going to keep this comment short so that I can mid-air Ilana, who probably also has some thoughts.  ;)
Attachment #8526069 - Flags: feedback?(bwinton) → feedback+
Comment on attachment 8526069 [details] [diff] [review]
switch telemetry to use an array of strings to indicate all states,

Review of attachment 8526069 [details] [diff] [review]:
-----------------------------------------------------------------

Are there no tests that need to be updated? If not, we should probably add some.
Attachment #8526069 - Flags: review?(jaws) → review-
Comment on attachment 8526069 [details] [diff] [review]
switch telemetry to use an array of strings to indicate all states,

Review of attachment 8526069 [details] [diff] [review]:
-----------------------------------------------------------------

Discussed over IRC. We need tests on the server side to make sure that they are parsing and displaying the data correctly. Probably introduce a sort of Talos-style email alert if large variations in the data are seen. Gijs will be filing a bug for this. Changing review status to r+ accordingly.
Attachment #8526069 - Flags: review- → review+
https://hg.mozilla.org/integration/fx-team/rev/35f07c2d524b

Filed bug 1104368 about the telemetry alerts
Flags: needinfo?(jaws)
Iteration: 36.3 → 37.1
https://hg.mozilla.org/mozilla-central/rev/35f07c2d524b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
QA Contact: catalin.varga
Hi Gij I've tested the scenario described in the bug on the latest Nightly and it's not reproducing anymore but I was wondering if that's enough to verify the bug, as this fix seems more complex?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Catalin Varga [QA][:VarCat] from comment #16)
> Hi Gij I've tested the scenario described in the bug on the latest Nightly
> and it's not reproducing anymore but I was wondering if that's enough to
> verify the bug, as this fix seems more complex?

It would make sense to verify that the current behaviour is correct, ie the right arrays get logged in telemetry if you click in ways that satisfy more than one of the criteria (e.g. right click on a video while having a selection). If that is not clear enough, maybe check with Blake (:bwinton) about the best way to verify the fix here.
Flags: needinfo?(gijskruitbosch+bugs)
Verified as fixed using:

FF 36
Build Id: 20150104004008
OS: Win 7 x64.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.