UI Telemetry events with null methods are ignored

RESOLVED FIXED in Firefox 31

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

Trunk
Firefox 33
All
Android
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox30 wontfix, firefox31 fixed, firefox32 fixed, firefox33 fixed, firefox-esr24 unaffected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 8443249 [details] [diff] [review]
fix-null-method v0.1

Looks like a copy/paste error and the tests didn't cover the error case, plus the tests were too one-sided.

When we build the event we put the action into "data" and method into "characters":
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoEvent.java#818

We check for "characters", but that is method, so we ignore it.
http://mxr.mozilla.org/mozilla-central/source/widget/android/nsAppShell.cpp#425

The tests didn't have a "method = null" case. Even if they did, the primary loop was over the UITelemetry.getUIMeasurement array, which didn't have the missing row. If the test was the last row in the expected array, it would never have been checked. (Took me a few minutes to figure that out).

This patch:
* Checks the length of Data
* Converts a non-unique event test into a null method event test
* Nulls are converted to empty strings at some point in the JSON stringification, so the METHOD_NULL const is ""
* Test both UITelemetry and expected loops as primary. It's the newly added loop that catches the error.
Attachment #8443249 - Flags: review?(rnewman)
Several of our LOAD_URL probes currently use null methods and are being ignored.
Comment on attachment 8443249 [details] [diff] [review]
fix-null-method v0.1

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

Check is fine, but tests need a tweak.

::: mobile/android/base/tests/testUITelemetry.java
@@ +43,5 @@
>              // This session is already stopped, so this call should be ignored.
>              Telemetry.stopUISession(Session._TEST_STOPPED_TWICE, Reason._TEST_IGNORED);
>  
> +            // Method can be null
> +            Telemetry.sendUIEvent(Event._TEST1);

Strictly speaking:

  // Method defaults to Method.NONE.

::: mobile/android/base/tests/testUITelemetry.js
@@ +15,5 @@
>  const EVENT_TEST4 = "_test_event_4.1";
>  
>  const METHOD_TEST1 = "_test_method_1";
>  const METHOD_TEST2 = "_test_method_2";
> +const METHOD_NULL = "";

This should really be

  const METHOD_NONE = null;

Method.NONE has a null string, and we expect that to propagate.

@@ +101,5 @@
>  
> +  expected.forEach(function (m, i) {
> +    if (m.type === "event") {
> +      m.sessions = removeNonTestSessions(m.sessions);
> +      m.sessions.sort(); // Mutates.

There should never be test sessions in the expected values (m). You might want to do this for measurements[i], though.
Assignee: nobody → mark.finkle
Status: NEW → ASSIGNED
OS: Linux → Android
Hardware: x86_64 → All
(In reply to Richard Newman [:rnewman] from comment #2)

> > +const METHOD_NULL = "";
> 
> This should really be
> 
>   const METHOD_NONE = null;
> 
> Method.NONE has a null string, and we expect that to propagate.

But the JSON stringification somehow converts any | null | (for Reason or Method) to "", so we need to test against "" and not null. I started with null and the tests failed.

> * Nulls are converted to empty strings at some point in the JSON stringification, so the METHOD_NULL const is ""

> > +  expected.forEach(function (m, i) {
> > +    if (m.type === "event") {
> > +      m.sessions = removeNonTestSessions(m.sessions);
> > +      m.sessions.sort(); // Mutates.
> 
> There should never be test sessions in the expected values (m). You might
> want to do this for measurements[i], though.

Technically it's already done in the first pass. I can remove this copy/paste code.
(In reply to Mark Finkle (:mfinkle) from comment #3)

> > * Nulls are converted to empty strings at some point in the JSON stringification, so the METHOD_NULL const is ""

Aieeee

Let's explain that in a comment. I think the translation is actually in the event passing, not in JSON-land.

> Technically it's already done in the first pass. I can remove this
> copy/paste code.

Yeah, I was thinking that... unless new events get recorded while the test is running!
Created attachment 8443520 [details] [diff] [review]
fix-null-method v0.2

Removes the expected event sanitization
Adds a comment about "" vs null
Rename METHOD_NULL to METHOD_NONE

Test pass with fix. Test fail without fix.
Attachment #8443249 - Attachment is obsolete: true
Attachment #8443249 - Flags: review?(rnewman)
Attachment #8443520 - Flags: review?(rnewman)
Attachment #8443520 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/776f6d341b3f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment on attachment 8443520 [details] [diff] [review]
fix-null-method v0.2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 932092
User impact if declined: We are missing large chunks of LOADURL telemetry
Testing completed (on m-c, etc.): landed with tests passing
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8443520 - Flags: approval-mozilla-beta?
Attachment #8443520 - Flags: approval-mozilla-aurora?
Marking as affected in 30-33 based on bug 932092 having been fixed on 29.
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → fixed
status-firefox-esr24: --- → unaffected
Comment on attachment 8443520 [details] [diff] [review]
fix-null-method v0.2

Aurora approval granted. 

For Beta, given that this issue exists in 29 and 30, how important is it that we fix this on 31?
Attachment #8443520 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Lawrence Mandel [:lmandel] from comment #10)

> For Beta, given that this issue exists in 29 and 30, how important is it
> that we fix this on 31?

Our current UI decision making is reliant on having as much data as we can, as soon as we can get it -- we got fed up of flying blind. That's the driver for us uplifting telemetry patches almost routinely.

Beta is where we start to get reliable data (larger population), and of course the same applies to reaching release six weeks sooner.

The only reason *not* to uplift telemetry patches as far as we can is risk management.

Two factors make me very confident in uplifting this: the small size and nature of the patch, and the fact that we'll get almost immediate feedback (Aurora at first, Beta next) to verify that it worked and didn't regress anything -- we'll start getting more data in telemetry, and no new crashes. There's time to back out if the risk assessment was wrong.
Attachment #8443520 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
On Beta we have an unmet dependency on Bug 1009315, which isn't trivial to land, so I uplifted this without the test changes.

https://hg.mozilla.org/releases/mozilla-beta/rev/400fbdb57bca
status-firefox30: affected → wontfix
status-firefox31: affected → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.