1247489 - Move TelemetryPing construction to a Builder

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

(This is still getting finalized in my head but...)

As we add more arguments, it gets difficult to manage so many, particularly when some arguments are optional and we need several overloaded methods in order to provide the functionality.

As such, we should replace TelemetryPingGenerator.create*Ping with a TelemetryPingBuilder class.

In my perceived implementation, we should build TelemetryPing in the calling code, serializing it to pass it through the Intent. This allows the TelemetryUploadService to focus solely on uploading. Another benefit is that we can throw if the ping is improperly formatted (e.g. invalid profile path) when it is built, rather than waiting for the upload service to build & validate the ping

Some cons:
* The caller has to know more about how to manage the profile to get out client IDs & such. We might be able to mitigate this by having the builder take in a profile and doing the magic, which is simpler than the solution we have now where the profile is accessed by the Intent?
* Caller has to know what makes a properly formatted ping of each type
* Each time we add a field, we may have to update the Parcelable methods (which allow us to send the Ping in an Intent).

It'll be important to consider this in relation to bug 1243585, e.g. maybe instead of sending it through the Intent, we can serialize it to disk and access it from disk in the UploadService. Storing it on disk is important because I don't think there's any other way we can atomically create & save a TelemetryPing and reset the counts we'll be taking for other fields in the ping.

Comment 1

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

Attachment: MozReview Request: Bug 1247489 - Replace TelemetryPingGenerator with TelemetryPingBuilder & friends. r=grisha

The Builder pattern has the following benefits:
  * Encapsulate identifying optional arguments
  * Encapsulate parameter validation
  * More fluent parameter insertion (e.g. instead as unnamed arguments to a
function)
  * My implementation makes it fairly straight-forward to construct new
telemetry pings.

Review commit: https://reviewboard.mozilla.org/r/44217/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44217/

Comment 2

[:Grisha Kruglov]

Comment on attachment 8737969 [details]
MozReview Request: Bug 1247489 - Replace TelemetryPingGenerator with TelemetryPingBuilder & friends. r=grisha

https://reviewboard.mozilla.org/r/44217/#review41811

A few thoughts:
- maybe `TelemetryPing.CoreBuilder` would be a more user-friendly place from a discoverability point of view?
- what does the Contract inner class win us? I'd move those constants into the parent class for simplicity
- perhaps `set*` vs `add*`? e.g. `setDefaultSearchEngine` vs `addDefaultSearchEngine`? The latter implies optionality, which isn't the case. Personally I think `set` reads better, but I suppose that's a matter of taste
- there's bunch of argument validation as well as overall payload validation; perhaps junit4 tests for that stuff would be good?

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryCorePingBuilder.java:102
(Diff revision 1)
> +        }
> +        payload.put(Contract.PROFILE_CREATION_DATE, date);
> +        return this;
> +    }
> +
> +    // TODO: We can potentially build two pings with the same seq no if we leave seq as an argument.

How much do we care? Perhaps make a bug for this?

Comment 3

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

https://reviewboard.mozilla.org/r/44217/#review41811

> - maybe TelemetryPing.CoreBuilder would be a more user-friendly place from a discoverability point of view?

The hierarchy makes sense for discoverability but adding such a large class to the file will make it look like a TelemetryPing object is more complicated than it really is. Perhaps having all of the `*Ping*` in its own package would alleviate this issue by making it obvious there are builders? e.g. `telemetry/pings/*Ping*`. What do you think?

I also added a comment to `TelemetryPing` to look at the builder.

> - what does the Contract inner class win us? I'd move those constants into the parent class for simplicity

Absolutely nothing! A copy-pasta mistake. I'll remove it.

> - perhaps set* vs add*? e.g. setDefaultSearchEngine vs addDefaultSearchEngine? The latter implies optionality, which isn't the case. Personally I think set reads better, but I suppose that's a matter of taste

Yeah, I like set – I'll fix it.

> - there's bunch of argument validation as well as overall payload validation; perhaps junit4 tests for that stuff would be good?

Yeah, it'd be good to test this to prevent regressions.

> How much do we care? Perhaps make a bug for this?

I intend on editing this code some more for bug 1243585 so this is mostly a note to self not to forget about this potential issue! I don't think it warrants a bug. That being said, I added my nick so at least someone can ask me what it is if they see it at a later point.

Comment 4

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

Attachment: MozReview Request: Bug 1247489 - Move Telemetry*Ping* to telemetry/pings pkg. r=grisha

This should help make the Builders more discoverable when looking at the
TelemetryPing class.

Review commit: https://reviewboard.mozilla.org/r/45641/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45641/

Comment 5

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

Attachment: MozReview Request: Bug 1247489 - Add tests for mandatory field validation in TelemetryPingBuilder. r=grisha

Review commit: https://reviewboard.mozilla.org/r/45643/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45643/

Comment 6

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

Comment on attachment 8737969 [details]
MozReview Request: Bug 1247489 - Replace TelemetryPingGenerator with TelemetryPingBuilder & friends. r=grisha

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44217/diff/1-2/

Comment 7

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

Comment on attachment 8740228 [details]
MozReview Request: Bug 1247489 - Add tests for mandatory field validation in TelemetryPingBuilder. r=grisha

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45643/diff/1-2/

Comment 8

[:Grisha Kruglov]

Comment on attachment 8737969 [details]
MozReview Request: Bug 1247489 - Replace TelemetryPingGenerator with TelemetryPingBuilder & friends. r=grisha

https://reviewboard.mozilla.org/r/44217/#review42467

lgtm

Comment 9

[:Grisha Kruglov]

Comment on attachment 8740227 [details]
MozReview Request: Bug 1247489 - Move Telemetry*Ping* to telemetry/pings pkg. r=grisha

https://reviewboard.mozilla.org/r/45641/#review42469

lgtm

Comment 10

[:Grisha Kruglov]

Comment on attachment 8740228 [details]
MozReview Request: Bug 1247489 - Add tests for mandatory field validation in TelemetryPingBuilder. r=grisha

https://reviewboard.mozilla.org/r/45643/#review42473

Up to you, but perhaps also add tests for some specifics in TelemetryCorePingBuilder? Around which fields are mandatory and some specific validations (e.g. nullable but non-empty engine, for example);

Comment 11

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

https://reviewboard.mozilla.org/r/45643/#review42473

I think this would be overkill – if the schema ever changes, the tests will have to change too. We also have server-side schema validation to cover this. I'm going to leave it as is.

Thanks for the thorough review! :)

Comment 12

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

Mike, can you let me know when you land your distribution ping changes land? I'd like to land this but I don't want to cause you to rewrite your patch. I think it'd be more efficient to rewrite from my side anyway.

Comment 13

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

The distribution changes landed.

Comment 14

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

https://hg.mozilla.org/integration/fx-team/rev/59c84a2beabdb13a97f181ba3fcdaf327357d6cb
Bug 1247489 - Replace TelemetryPingGenerator with TelemetryPingBuilder & friends. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/c2ad1f090a2ee81dc50af2f0dd87151923329075
Bug 1247489 - Move Telemetry*Ping* to telemetry/pings pkg. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/9fdf15002094efa7092857de08608c36350b6413
Bug 1247489 - Add tests for mandatory field validation in TelemetryPingBuilder. r=grisha

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/59c84a2beabd
https://hg.mozilla.org/mozilla-central/rev/c2ad1f090a2e
https://hg.mozilla.org/mozilla-central/rev/9fdf15002094