1135791 - Add a new unified ping type: "saved-session"

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Vladan Djeric (:vladan)]

Currently, the unified Telemetry code sets saved-session pings' ping.type field to "main". However, these pings are different from other "main" pings (daily, environment-changed, shutdown) and they will be stored separately from other "main" pings in S3. 

Additionally, the backend code expects the ping's type to match the ping's submission URL and the ping's S3 storage bucket.  

To remain consistent, the saved-session pings should have ping.type = "saved-session" and the submission path should be "../saved-session/..", not "../main/.."

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1135791.patch

This patch sets the main ping reason to "saved-session" for payloads with that reason.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8568595 [details] [diff] [review]
bug1135791.patch

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

This is missing 3 places to adjust in TelemetrySession.jsm
Search for PING_TYPE_MAIN through the file: savePendingPings, testSaveHistograms, "application-background".

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +991,5 @@
>      };
> +    // To remain consistent with server-side ping handling, set "saved-session" as the ping
> +    // type for "saved-session" payload reasons.
> +    let pingType =
> +      (payload.info.reason == REASON_SAVED_SESSION) ? PING_TYPE_SAVED_SESSION : PING_TYPE_MAIN;

Nit: That would be much more readable with either:
* a temp variable: const isSavedSession = ...
* different branching: let pingType = PING_TYPE_MAIN; if (reason == ...) ...
* a helper function like getPingType(payload)

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1135791.patch - v2

Thanks for the review. This patch adds an helper function to get the ping type and uses that function every time a ping must be sent.

test_TelemetrySession.js is updated as well.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8568627 [details] [diff] [review]
bug1135791.patch - v2

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

::: toolkit/components/telemetry/docs/common-ping.rst
@@ +16,5 @@
>  
>  Structure::
>  
>      {
> +      type: <string>, // "main", "activation", "deletion", "saved-session", ...

Please update pings.rst too - document the type there and that we use it to make storage of saved-session easier server-side.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: bug1135791.patch - v3

Updated ping.rst.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Pushed in: https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=b83c3fa8c80f

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d6d1af5f57eb