Closed
Bug 1135791
Opened 10 years ago
Closed 10 years ago
Add a new unified ping type: "saved-session"
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: vladan, Assigned: Dexter)
References
Details
(Whiteboard: [ready])
Attachments
(1 file, 2 obsolete files)
10.68 KB,
patch
|
Dexter
:
review+
gfritzsche
:
feedback+
|
Details | Diff | Splinter Review |
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/.."
Assignee | ||
Comment 1•10 years ago
|
||
This patch sets the main ping reason to "saved-session" for payloads with that reason.
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8568595 -
Flags: review?(gfritzsche)
Comment 2•10 years ago
|
||
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)
Attachment #8568595 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 3•10 years ago
|
||
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.
Attachment #8568595 -
Attachment is obsolete: true
Attachment #8568627 -
Flags: review?(gfritzsche)
Comment 4•10 years ago
|
||
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.
Attachment #8568627 -
Flags: review?(gfritzsche) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Updated ping.rst.
Attachment #8568627 -
Attachment is obsolete: true
Attachment #8568725 -
Flags: review+
Attachment #8568725 -
Flags: feedback?(gfritzsche)
Updated•10 years ago
|
Attachment #8568725 -
Flags: feedback?(gfritzsche) → feedback+
Updated•10 years ago
|
Whiteboard: [ready]
Comment 6•10 years ago
|
||
Comment 7•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•