Closed Bug 1120981 Opened 5 years ago Closed 5 years ago

Properly specify the common ping format

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 1069869
Blocks: 1120982
Attached file Common ping format, v1 (obsolete) —
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)
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.
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?
(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.
Attached file Common ping format, v2 (obsolete) —
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)
Blocks: 1122061
bsmedberg suggests to include:
* type
* ping id
* the build information from the environment
* the update channel
* profile id (optional)
* environment (optional)
Attached file Common ping format, v3 (obsolete) —
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 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+
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.
(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 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+
(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.
Attached file 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)
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.
Blocks: 1124212
(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.
This part of the design seems to be fine, closing.
Status: ASSIGNED → RESOLVED
Closed: 5 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 → ---
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)
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
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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 → ---
Anything we do with that should be the session ping, not the common ping.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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.
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.
Fair enough, works for me.
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.