Properly specify the common ping format

RESOLVED FIXED

Status

Firefox Health Report
Client: Desktop
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
We need to properly specify the common ping format so that this isn't just incidental.
This should be documented properly, possibly with in-tree documentation.
(Assignee)

Updated

3 years ago
Blocks: 1069869
(Assignee)

Updated

3 years ago
Blocks: 1120982
(Assignee)

Comment 1

3 years ago
Created attachment 8548936 [details]
Common ping format, v1

Assumptions:
* added a version field, just holding 1 for now - in case we ever need to bump this, it will be painful if we didn't add it before
* both client id & environment are optional. This is for privacy and to not submit more then we need to.
* environment.data is optional as we probably don't want to resubmit env data that we sucessfully send out?
* ping date should be useful to tell for all ping types when it was generated
Assignee: nobody → gfritzsche
Status: NEW → ASSIGNED
Attachment #8548936 - Flags: feedback?(vdjeric)
Attachment #8548936 - Flags: feedback?(rvitillo)
Attachment #8548936 - Flags: feedback?(mreid)
Attachment #8548936 - Flags: feedback?(benjamin)
Attachment #8548936 - Flags: feedback?(bcolloran)
(Assignee)

Comment 2

3 years ago
Per bug 1120982 etc., we would also like to rename the "reason" field into "type" (or "name" or ...).
"type" would e.g. be "main" for the normal telemetry pings, "cert-violation" for update pings etc.
Then we would have a "reason" field in the "main" telemetry payload which would be the reason that triggered that main ping, like the current "idle-daily", "saved-session", etc.

Comment 3

3 years ago
In the design doc here
https://docs.google.com/a/mozilla.com/document/d/1IGpzsYGi_sq3YFQDAPyKOkU_BKvXAC95fZYA2i4ceVs/edit#
I thought we had discussed having the top-level ping include basic facets equivalent to what is included in the blocklist. In that doc it's described as:

{
  “type”: “environment”,
  “pingid”: “1234579-uuid”,
  “application”: “Firefox”,
  “appVersion”: “36.0”,
  “channel”: “beta”,
  “buildid”: “12345017324”,
  “architecture”: “WINNT”,
  “profile”: “12345789-uuid”, // optional, some pings will not contain this data
  “environment”: { … see below }, // optional, some pings will not contain this data
  “data”: { … }
}

I think the idea was that in order to do sanity checking every ping should minimally be comparable to BLP bins, but I cannot recall the details. This would create redundancy with environment, but since not all pings will have that (cert ping, maybe activation ping, others?) it would be good to include these most basic bits of info for all pings. Has the thinking on this topic moved on since then?
(Assignee)

Comment 4

3 years ago
(In reply to brendan c from comment #3)
> I think the idea was that in order to do sanity checking every ping should
> minimally be comparable to BLP bins, but I cannot recall the details. This
> would create redundancy with environment, but since not all pings will have
> that (cert ping, maybe activation ping, others?) it would be good to include
> these most basic bits of info for all pings. Has the thinking on this topic
> moved on since then?

Thanks for raising this - it was missed and we need to revise this.
Created attachment 8549555 [details]
Common ping format, v2

Updated to address the comments from Brendan.
Attachment #8548936 - Attachment is obsolete: true
Attachment #8548936 - Flags: feedback?(vdjeric)
Attachment #8548936 - Flags: feedback?(rvitillo)
Attachment #8548936 - Flags: feedback?(mreid)
Attachment #8548936 - Flags: feedback?(benjamin)
Attachment #8548936 - Flags: feedback?(bcolloran)
Attachment #8549555 - Flags: feedback?(vdjeric)
Attachment #8549555 - Flags: feedback?(rvitillo)
Attachment #8549555 - Flags: feedback?(mreid)
Attachment #8549555 - Flags: feedback?(benjamin)
Attachment #8549555 - Flags: feedback?(bcolloran)
(Assignee)

Updated

3 years ago
Blocks: 1122061
(Assignee)

Comment 6

3 years ago
bsmedberg suggests to include:
* type
* ping id
* the build information from the environment
* the update channel
* profile id (optional)
* environment (optional)
Created attachment 8549742 [details]
Common ping format, v3

What changed:

* Commented the pingId field;
* Enriched the "application" section with more information to identify the build; this would likely help with the operational aspects of server side storage.
Attachment #8549555 - Attachment is obsolete: true
Attachment #8549555 - Flags: feedback?(vdjeric)
Attachment #8549555 - Flags: feedback?(rvitillo)
Attachment #8549555 - Flags: feedback?(mreid)
Attachment #8549555 - Flags: feedback?(benjamin)
Attachment #8549555 - Flags: feedback?(bcolloran)
Attachment #8549742 - Flags: feedback?(vdjeric)
Attachment #8549742 - Flags: feedback?(rvitillo)
Attachment #8549742 - Flags: feedback?(mreid)
Attachment #8549742 - Flags: feedback?(benjamin)
Attachment #8549742 - Flags: feedback?(bcolloran)

Comment 8

3 years ago
Comment on attachment 8549742 [details]
Common ping format, v3

The current ping version is 1, we should probably use 2. Although it will be at a different URL endpoint certainly, so we can distinguish them regardless.

We've used seconds-since-epoch as timestamps before, is there a reason we're using an ISO date here?
Attachment #8549742 - Flags: feedback?(benjamin) → feedback+

Comment 9

3 years ago
The Metrics Team requests that we use ISO dates whenever possible. Human-readability should always be preferred to arcane encoding unless there is a very good reason. This *does* make a real difference when working with the data.

Comment 10

3 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Comment on attachment 8549742 [details]
> Common ping format, v3
> 
> The current ping version is 1, we should probably use 2. Although it will be
> at a different URL endpoint certainly, so we can distinguish them regardless.

If you're talking about current Telemetry payload version (the "ver" field), there are a few others in use on the server-side and by other clients. They are listed here:
https://github.com/mozilla/telemetry-server/blob/master/telemetry/convert.py#L37-L39

Specifically, we use ver=2 to identify payloads whose Histograms have been converted to the array-style format described here: https://github.com/mozilla/telemetry-server/blob/master/docs/StorageFormat.md#example

And ver=3 to handle a quirk of FxOS 1.3 that we couldn't easily patch on the client.

So if we want to maintain consistency with that scheme, we should use version 4 (though that might be confusing with the "FHRv4" planning, so we could use something else, as long as it's not 1, 2, or 3).

Comment 11

3 years ago
Comment on attachment 8549742 [details]
Common ping format, v3

>  pingId: <UUID>, // a UUID that identifies this ping
>  pingDate: <ISO date>, // the date the ping was generated
I don't find "ping" a useful field prefix - something like "documentId" and "generationDate" would convey more information.

Other than that, this looks good to me.
Attachment #8549742 - Flags: feedback?(mreid) → feedback+
(Assignee)

Comment 12

3 years ago
(In reply to Mark Reid [:mreid] from comment #10)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> > Comment on attachment 8549742 [details]
> > Common ping format, v3
> > 
> > The current ping version is 1, we should probably use 2. Although it will be
> > at a different URL endpoint certainly, so we can distinguish them regardless.
> 
> If you're talking about current Telemetry payload version (the "ver" field),
> there are a few others in use on the server-side and by other clients.

Currently the ping doesn't have a version field on the top-level. We mean to introduce a new "ping version", which is separate from the various payload versions.
Created attachment 8550216 [details]
Common ping format, v4

Thank you for your input! This revision renames |pingId|, |pingDate| and changes the value of the version field to 2.

If there's no additional input on this, we would like to go with this revision.
Attachment #8549742 - Attachment is obsolete: true
Attachment #8549742 - Flags: feedback?(vdjeric)
Attachment #8549742 - Flags: feedback?(rvitillo)
Attachment #8549742 - Flags: feedback?(bcolloran)

Comment 14

3 years ago
As mentioned in comment 10 here: https://bugzilla.mozilla.org/show_bug.cgi?id=1120982#c10 -- should creationDate be a timestamp? I don't think it should be necessary since we'll capture that detail in the session fragment ping environment, but I guess that level of detail could move out of the environment and up to the common ping wrapper as long as it ends up somewhere.
(Assignee)

Updated

3 years ago
Blocks: 1124212
(Assignee)

Comment 15

3 years ago
(In reply to brendan c from comment #14)
> As mentioned in comment 10 here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1120982#c10 -- should
> creationDate be a timestamp? I don't think it should be necessary since
> we'll capture that detail in the session fragment ping environment, but I
> guess that level of detail could move out of the environment and up to the
> common ping wrapper as long as it ends up somewhere.

creationDate here covers when the ping was created.
The environment is optional on pings and the payload may differ between ping types.
(Assignee)

Comment 16

3 years ago
This part of the design seems to be fine, closing.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
I think FHR analyses (which are based on calendar day, local time) will need the local calendar date reported as well in each sub-session ping in order to know when the measurements were taken (local time).
Flags: needinfo?(bcolloran)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 18

3 years ago
My understanding was that the sessionStartDate timestamp in the main ping [1] (which will be recorded in local time, and submitted with each session fragment) will cover our needs. Is this not correct?

You're right, this is super important for FHR use cases -- if you can see a reason this timestamp will not be sufficient, please elaborate. Thanks!

[1] see https://bugzilla.mozilla.org/show_bug.cgi?id=1120982#c13
Flags: needinfo?(bcolloran)
(Assignee)

Comment 19

3 years ago
We had a misunderstanding on IRC. Comment 17 was about the session payloads and thus its already handled by what comment 18 points to.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Comment 20

3 years ago
From https://bugzilla.mozilla.org/show_bug.cgi?id=1120982#c31 --
I think we should add a "previousPingId" field (or "previousId" or whatever, I don't care about the name)

> > Brendan has a different suggestion in comment 22. I'm not sure if we need
> > both or which is more useful server-side.
> 
> I think there's value to FHR to having both. The persistent count across
> session will sometimes be incorrect (copied profiles, browser crashes,
> Firefox pruning old pings, etc) but I think that's why Brendan wants it in
> the first place.

FTR, the use case I'm talking about could be better served by adding a "previousPingId" field to the common ping ( https://bugzilla.mozilla.org/show_bug.cgi?id=1120981 ). This would allow unambiguous reconstruction of ping trees in cases of copying/vms/rollback etc. Having the data structured in this way would really help us get a handle on the frequency of these behaviors in our user population.

So I guess I would favor: session fragment numbering per session to serve Vladan's use case, and pointers to previous pings to serve the ping chain/tree reconstruction and data auditing use case.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 21

3 years ago
Anything we do with that should be the session ping, not the common ping.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Comment 22

3 years ago
But the pingId lives in the common ping, no?

Anyway I don't really care. If we want to add a sessionId + previousSessionId in the session ping, that is functionally equivalent.

Comment 23

3 years ago
The pingID lives in the common ping.

Any "previousSessionPing" field would be specific to session pings, though. We don't want to be able to build a history of any type of ping, just limit the "tree" to sessions.

Comment 24

3 years ago
Fair enough, works for me.
You need to log in before you can comment on or make changes to this bug.