Closed Bug 1009315 Opened 6 years ago Closed 5 years ago

Replace TelemetryContract interfaces with enums

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(4 files, 6 obsolete files)

4.65 KB, patch
liuche
: review+
Details | Diff | Splinter Review
4.55 KB, patch
liuche
: review+
Details | Diff | Splinter Review
20.69 KB, patch
liuche
: review+
Details | Diff | Splinter Review
13.35 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
This allows type-checking for Telemetry.* methods (e.g. `Telemetry.sendUIEvent(Event, Method, ...)`).
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Finkle recommended doing this while I was committing to all of this churn anyway.
Attachment #8423329 - Flags: review?(liuche)
Attachment #8423329 - Attachment is obsolete: true
Attachment #8423329 - Flags: review?(liuche)
Attachment #8423327 - Flags: review?(liuche) → review+
Test fixes in part 4.
Attachment #8423441 - Flags: review?(liuche)
Attachment #8423331 - Flags: review?(liuche) → review+
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 on attachment 8423327 [details] [diff] [review]
Part 1: Sort TelemetryContract items.

Actually rnewman already landed this in bug 917480, part 5.
Attachment #8423327 - Attachment is obsolete: true
Comment on attachment 8423327 [details] [diff] [review]
Part 1: Sort TelemetryContract items.

Er, part of this patch, anyways.
Attachment #8423327 - Attachment is obsolete: false
(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. :)
Attached patch Part 4: Fix testUITelemetry. (obsolete) — Splinter Review
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
Attachment #8423502 - Flags: review?(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!
Attachment #8423441 - Flags: review?(liuche) → review+
Blocks: 979443
Fixed the NullPointerExceptions in the Try push from comment 9 - I used null instead of Method.NONE.
Attachment #8424064 - Flags: review?(liuche)
Attachment #8423441 - Attachment is obsolete: true
No longer blocks: 979443
Attachment #8423502 - Attachment is obsolete: true
Attachment #8423502 - Flags: review?(liuche)
Attachment #8429723 - Attachment is obsolete: true
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?
Attachment #8424064 - Flags: review?(liuche) → review+
(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.
Attached patch Part 4: Fix testUITelemetry. (obsolete) — Splinter Review
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
Attachment #8431733 - Flags: review?(liuche)
Attachment #8431092 - Attachment is obsolete: true
Attachment #8431092 - Flags: review?(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.
Attachment #8431733 - Flags: review?(liuche) → review+
(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.
Attachment #8431733 - Attachment is obsolete: true
Depends on: 1019158
Forgot to remove the leave-open tag.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.