1009315 - Replace TelemetryContract interfaces with enums

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This allows type-checking for Telemetry.* methods (e.g. `Telemetry.sendUIEvent(Event, Method, ...)`).

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Sort TelemetryContract items.

Details...

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Refactor top sites probe names to be generic.

Finkle recommended doing this while I was committing to all of this churn anyway.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Refactor top sites probe names to be generic.

OCD comment fixes.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Replace TelemetryContract interfaces with enums.

Test fixes in part 4.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

re part 3: I should note that accomodating for `":" + String` over the Enum makes the Telemetry API a bit more messy, but I couldn't come up with another solution.

One possible thought is to create Wrapper Objects that take the enum instance and the suffix String and pass this to the Telemetry methods, but the extra allocation is undesirable. One way around this is to create an Object pool, but at that point, I think it's much too overengineered.

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8423327 [details] [diff] [review]
Part 1: Sort TelemetryContract items.

Actually rnewman already landed this in bug 917480, part 5.

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8423327 [details] [diff] [review]
Part 1: Sort TelemetryContract items.

Er, part of this patch, anyways.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Chenxia Liu [:liuche] from comment #6)
> Actually rnewman already landed this in bug 917480, part 5.

He landed the churn for Event - I did the others.

I consider it inspiration. :)

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 4: Fix testUITelemetry.

I spent more time fixing this than I would like so it's a bit hacky - feel free
to send me back to make this more robust.

try: https://tbpl.mozilla.org/?tree=Try&rev=11461f595832

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8423441 [details] [diff] [review]
Part 3: Replace TelemetryContract interfaces with enums.

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

Nice, this looks good. I wouldn't worry about the session/event concatenation - I'm sorry it's so ugly!

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Replace TelemetryContract interfaces with enums.

Fixed the NullPointerExceptions in the Try push from comment 9 - I used null instead of Method.NONE.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

try (again): https://tbpl.mozilla.org/?tree=Try&rev=75c6b741c9a9

Comment 13

[Chenxia Liu [:liuche]]

Landing Parts 1 & 2 for bug 1013672.

https://hg.mozilla.org/integration/fx-team/rev/5ccd250f580e
https://hg.mozilla.org/integration/fx-team/rev/7f1171701401

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ccd250f580e
https://hg.mozilla.org/mozilla-central/rev/7f1171701401

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 4: Fix testUITelemetry.

try (try again): https://tbpl.mozilla.org/?tree=Try&rev=0cc962c00bd5

Will flag for review if green.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 4: Fix testUITelemetry.

try(ing too hard?): https://tbpl.mozilla.org/?tree=Try&rev=b088bc6c6899

Comment 17

[Chenxia Liu [:liuche]]

Comment on attachment 8424064 [details] [diff] [review]
Part 3: Replace TelemetryContract interfaces with enums.

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

Looks good.

::: mobile/android/base/Telemetry.java
@@ +127,5 @@
> +    public static void stopUISession(final Session session, final String sessionNameSuffix,
> +            final Reason reason) {
> +        final String sessionName = getSessionName(session, sessionNameSuffix);
> +
> +        Log.d(LOGTAG, "StopUISession: " + sessionName + ", reason=" + reason);

I assume this works fine with the NONE(null) enum? Nothing silly like null.toString() getting called to make the log message?

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Chenxia Liu [:liuche] from comment #17)
> I assume this works fine with the NONE(null) enum? Nothing silly like
> null.toString() getting called to make the log message?

`NONE.toString` would be called, which only returns the value of `NONE.string`, which is `null`, so no, no silliness abound.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 4: Fix testUITelemetry.

As discussed on IRC, added test values to the TelemetryContract enums, rather
than keeping a set of String constants up to date with TelemetryContract in JS.
This also has the benefit of removing non-test measurements from our
assertions, making the test more robust.

green try: https://tbpl.mozilla.org/?tree=Try&rev=79dc706e4332

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8431733 [details] [diff] [review]
Part 4: Fix testUITelemetry.

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

Nice, just a few nits, all up to your judgment.

::: mobile/android/base/TelemetryContract.java
@@ +142,5 @@
> +        _TEST2("_test_method_2"),
> +        _TEST3("_test_method_3"),
> +        _TEST4("_test_method_4"),
> +        _TEST5("_test_method_5"),
> +        _TEST6("_test_method_6"),

Do we need this many method enums? Can we just reuse some of them?

That being said, I guess 2 test enums vs 6 test enums isn't a big deal.

::: mobile/android/base/tests/testUITelemetry.java
@@ +29,5 @@
> +            Telemetry.sendUIEvent(Event._TEST1, Method._TEST1);
> +            Telemetry.startUISession(Session._TEST_STARTED_TWICE);
> +            Telemetry.sendUIEvent(Event._TEST2, Method._TEST2);
> +
> +            // We can only start one session per name, so this call is ignored.

Maybe "so this call should be ignored" ?

@@ +39,5 @@
> +            Telemetry.stopUISession(Session._TEST_STARTED_TWICE, Reason._TEST1);
> +            Telemetry.sendUIEvent(Event._TEST4, Method._TEST5, "barextras");
> +            Telemetry.stopUISession(Session._TEST_STOPPED_TWICE, Reason._TEST2);
> +
> +            // This session is already stopped, so this call is ignored.

same

::: mobile/android/base/tests/testUITelemetry.js
@@ +34,5 @@
>    }
>  }
>  
> +/**
> + * Asserts that the given measurements are equal. Assumes that an measurements

typo: "an measurements"

@@ +37,5 @@
> +/**
> + * Asserts that the given measurements are equal. Assumes that an measurements
> + * of type "event" have their sessions arrays sorted.
> + */
> +function do_check_meas_eq(m1, m2) {

Nit: even slighter preference for measurement

@@ +54,5 @@
> +      do_check_eq(m1.reason, m2.reason);
> +      break;
> +
> +    default:
> +      do_throw("Unknown event type, " + m1.type);

Nit: No comma? "type:" or just "type".

@@ +130,5 @@
> +    }
> +  });
> +}
> +
> +function filterNonTestMeasurements(measurements) {

I'm not sure if filterX is intuitively filtering *out* something or applying a filter to allow everything else to pass. Maybe a comment, or maybe I'm just too easily confused.

@@ +131,5 @@
> +  });
> +}
> +
> +function filterNonTestMeasurements(measurements) {
> +  return measurements.filter(function (meas) {

Nit: Slight preference for measurement vs meas.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Chenxia Liu [:liuche] from comment #20)
> Do we need this many method enums? Can we just reuse some of them?

The idea was to ensure unique events for more specific testing, but yes, we can do that with 2.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 4: Fix testUITelemetry.

Updated nits.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/ee7ce47e5212
remote:   https://hg.mozilla.org/integration/fx-team/rev/c4c655e1f8eb

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ee7ce47e5212
https://hg.mozilla.org/mozilla-central/rev/c4c655e1f8eb

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Forgot to remove the leave-open tag.