1120981 - Properly specify the common ping format

Status: RESOLVED FIXED

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

Description

[Georg Fritzsche [:gfritzsche]]

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.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: 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

Comment 2

[Georg Fritzsche [:gfritzsche]]

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

[brendan c]

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?

Comment 4

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: Common ping format, v2

Updated to address the comments from Brendan.

Comment 6

[Georg Fritzsche [:gfritzsche]]

bsmedberg suggests to include:
* type
* ping id
* the build information from the environment
* the update channel
* profile id (optional)
* environment (optional)

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: 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.

Comment 8

[Benjamin Smedberg]

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?

Comment 9

[brendan c]

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

[Mark Reid [:mreid]]

(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

[Mark Reid [:mreid]]

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.

Comment 12

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: 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.

Comment 14

[brendan c]

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.

Comment 15

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 16

[Georg Fritzsche [:gfritzsche]]

This part of the design seems to be fine, closing.

Comment 17

[Vladan Djeric (:vladan)]

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).

Comment 18

[brendan c]

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

Comment 19

[Georg Fritzsche [:gfritzsche]]

We had a misunderstanding on IRC. Comment 17 was about the session payloads and thus its already handled by what comment 18 points to.

Comment 20

[brendan c]

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.

Comment 21

[Benjamin Smedberg]

Anything we do with that should be the session ping, not the common ping.

Comment 22

[brendan c]

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

[Benjamin Smedberg]

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

[brendan c]

Fair enough, works for me.