Closed Bug 1247489 Opened 8 years ago Closed 8 years ago

Move TelemetryPing construction to a Builder

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(3 files)

(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.
Blocks: core-ping
No longer blocks: ut-android
Assignee: nobody → michael.l.comella
Summary: Consider moving TelemetryPing construction to a Builder → Move TelemetryPing construction to a Builder
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/
Attachment #8737969 - Flags: review?(gkruglov)
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?
Attachment #8737969 - Flags: review?(gkruglov)
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.
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/
Attachment #8740227 - Flags: review?(gkruglov)
Attachment #8740228 - Flags: review?(gkruglov)
Attachment #8737969 - Flags: review?(gkruglov)
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 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 on attachment 8737969 [details]
MozReview Request: Bug 1247489 - Replace TelemetryPingGenerator with TelemetryPingBuilder & friends. r=grisha

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

lgtm
Attachment #8737969 - Flags: review?(gkruglov) → review+
Attachment #8740227 - Flags: review?(gkruglov) → review+
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
Attachment #8740228 - Flags: review?(gkruglov) → review+
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);
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! :)
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.
Flags: needinfo?(mozilla)
The distribution changes landed.
Flags: needinfo?(mozilla)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.