Restructure Telemetry client documentation

VERIFIED FIXED in Firefox 51

Status

()

P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

(Blocks: 1 bug)

Trunk
mozilla51
Points:
2

Firefox Tracking Flags

(firefox49 wontfix, firefox51 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
(From bug 1276201, comment #0)
> I think we should restructure the wiki to a structure like:
> * Adding/understanding Telemetry - provides overview and links to more
> specific pages:
>   * Adding new scalar measurements
>   * Adding new histograms (the current [0])
>   * Submitting custom pings
...
> 0: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/
> Adding_a_new_Telemetry_probe

Comment 1

3 years ago
I suggest that we move most/all of the functional docs (adding a new probe/scalar measurement/custom ping) to the in-tree docs.
(Assignee)

Comment 2

3 years ago
The overview & guide articles seem to work pretty well on the wiki.
They allow to quickly update them, even without the knowledge about the client patch work-flow, which seems valuable.

I agree that the more technical documentation lives better in-tree.
(Assignee)

Updated

3 years ago
Points: --- → 1
(Assignee)

Updated

3 years ago
Priority: P1 → P2
(Assignee)

Updated

3 years ago
Priority: P2 → P3
(Assignee)

Updated

3 years ago
Priority: P3 → P1
(Assignee)

Updated

3 years ago
Points: 1 → 2
(Assignee)

Comment 3

3 years ago
Given the bigger topic of more integrated data collection & pipeline docs, moving client documentation more into the in-tree docs makes sense.
We can morph the wiki article into a landing & overview page that links to those docs for details.

This means adding a lot more documentation pages though, so we need to restructure the client documentation first.
This drafts the potential structure and contents:
https://docs.google.com/document/d/1d9mqSYHtuq_s-wvlElQQEzPtDxAn2QjFyroejk-u8MY/
(Assignee)

Updated

3 years ago
Summary: Restructure MDN Telemetry documentation → Restructure Telemetry client documentation
(Assignee)

Updated

3 years ago
Assignee: nobody → gfritzsche
(Assignee)

Comment 4

3 years ago
Created attachment 8774415 [details] [diff] [review]
Part 1 - Restructure Telemetry client documentation

WIP. The structure works, i fixed a few build warnings. I can't figure out how to show more TOC depth in the sidebar, but that shouldn't be blocking here.
(Assignee)

Comment 5

3 years ago
Here is how it looks:
http://people.mozilla.org/~gfritzsche/tree-docs/toolkit/components/telemetry/telemetry/index.html

Does that look useful?
This is restructuring in the context of comment 3.
Flags: needinfo?(mreid)
Flags: needinfo?(chutten)
"Telemetry is a feature that allows data collection" -> "Telemetry is a Firefox feature that allows data collection"
"Note: the data collection policy documents the process and requirements that are applied here." -> "Note: the data collection policy documents the process and requirements for adding, removing, or changing data collection through these and other means"
"A Telemetry ping is the data that we send to Mozillas Telemetry servers." => Mozilla's

...there's a lot of spelling/grammar/style commentary I can put in here. Was that the sort of feedback you wanted, or were you just asking about the document structure?

From the structure POV, I'm a little annoyed that the subsections don't expand. If you click on "Concepts", there is no link in the sidebar for "Telemetry Pings"... I'm sure there is in the existing docs... Yeah, like here: http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/environment.html
Flags: needinfo?(chutten)
(Assignee)

Comment 7

3 years ago
(In reply to Chris H-C :chutten from comment #6)
> ...there's a lot of spelling/grammar/style commentary I can put in here. Was
> that the sort of feedback you wanted, or were you just asking about the
> document structure?

I'm not touching any content here, just restructuring and fixing references.
Feedback on the document structure is what i'm looking for.
(Assignee)

Comment 8

3 years ago
(In reply to Chris H-C :chutten from comment #6)
> From the structure POV, I'm a little annoyed that the subsections don't
> expand. If you click on "Concepts", there is no link in the sidebar for
> "Telemetry Pings"... I'm sure there is in the existing docs... Yeah, like
> here:
> http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/
> environment.html

Yes, that is not so great. With the restructuring there is now one more document nesting level.
This seems to handle a little complicated with sphinx here, i'm not sure yet how/if this can be restored.
(Assignee)

Comment 9

3 years ago
As it turns out, the sidebar nesting depth is only an issue locally (because our doc build doesn't quite match what RTD does).

Testing this on RTD, the sidebar nesting is fine:
http://gfritzsche-demo.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/concepts/pings.html

The RTD them supports a max nesting depth of 4, with apparently automatic expansion/collapsing for depths >2.
(Assignee)

Comment 10

3 years ago
Created attachment 8775116 [details] [diff] [review]
Part 1 - Restructure Telemetry client documentation

Fine-tuned some TOC handling.
(Assignee)

Updated

3 years ago
Attachment #8774415 - Attachment is obsolete: true
Comment on attachment 8775116 [details] [diff] [review]
Part 1 - Restructure Telemetry client documentation

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

Instead of the section "Data documentation", perhaps "Data transmission" or, more directly related to its contents, "Pings"

Having sections with only one subsection kinda sucks. From the design doc I presume we're intending to fill these out, but right now they're... eurgh.

Actually, looking at the design doc suggests that "Data documentation" may have to deal with more than just pings in the near future. I'm not sure about using the word "documentation" in a section header as it's pretty clear that's what all of this is, but I'll drop the pedantry.

f+ because this is my first time reviewing .rst documents.
Attachment #8775116 - Flags: feedback+
(Assignee)

Comment 12

3 years ago
This is part 1 with just the restructuring (otherwise this will become a fairly incomprehensible patch).

The idea is to have the "data documentation" section contain all the docs on data that we send out (environment, autogenerated histogram & scalar descriptions, ping docs, ...).
I'd be happy to hear better suggestions for the name here, i can't think of one.
I recommend making the distinction between "Measurement Types" and "Ping Types", where the former includes scalars, histograms, environment, etc; and the latter includes main, crash, sync, core, etc. 

I think those would make good section titles instead of "Data collection" and "Data documentation".

Then the "concepts" section could explain what we mean by Measurements vs. Pings.
Flags: needinfo?(mreid)
(Assignee)

Comment 14

3 years ago
Part of the motivation was a clear separation between (1) documenting data we send and (2) documenting mechanisms to do so.
The Firefox privacy notice [0] started to link to this documentation, so i think its valuable to keep this separate and digestible (and we could update the notice to directly link to the "data documentation").

[0] https://www.mozilla.org/en-US/privacy/firefox/
(Assignee)

Comment 15

3 years ago
Created attachment 8776652 [details] [diff] [review]
Part 2 - Flesh out docs/collection/

This sketches out the collection documentation. Its not aiming for perfection, we can fill this out more later.
Attachment #8776652 - Flags: feedback?(mreid)
(Assignee)

Comment 16

3 years ago
Created attachment 8776653 [details] [diff] [review]
Part 3 - Flesh out docs/concepts/

Ditto for the concepts documentation.
Attachment #8776653 - Flags: feedback?(mreid)
(Assignee)

Updated

3 years ago
Attachment #8776652 - Flags: feedback?(chutten)
(Assignee)

Updated

3 years ago
Attachment #8776653 - Flags: feedback?(chutten)
Comment on attachment 8776652 [details] [diff] [review]
Part 2 - Flesh out docs/collection/

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

::: toolkit/components/telemetry/docs/collection/custom-pings.rst
@@ +52,5 @@
> +- Data contents:
> +   - Does the submitted data answer the posed product questions?
> +   - Does the shape of the data allow to answer the questions efficiently?
> +   - Is the data limited to whats needed to answer the questions?
> +   - Does the data use common formats? (i.e. can we re-use tooling or analysis know-how)

Some of this is covered by the Data Collection policy link in the index. Do we want to reiterate that link here and say something like "in addition to <data collection link> you should answer questions like:"

::: toolkit/components/telemetry/docs/collection/measuring-time.rst
@@ +54,5 @@
> +      explicit AutoTimer(const nsCString& aKey,
> +                         TimeStamp aStart = TimeStamp::Now());
> +    };
> +
> +    void AccumulateTimeDelta(ID id, TimeStamp start, TimeStamp end = TimeStamp::Now());

No example?
Attachment #8776652 - Flags: feedback?(chutten) → feedback+
Comment on attachment 8776653 [details] [diff] [review]
Part 3 - Flesh out docs/concepts/

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

I can't help offering my unwanted content-based feedback :(

::: toolkit/components/telemetry/docs/concepts/archiving.rst
@@ +1,5 @@
> +=========
> +Archiving
> +=========
> +
> +When archiving is enabled through the relative preference, pings submitted to ``TelemetryController`` are also stored locally in the user profile directory, in ``<profile-dir>/datareporting/archived``.

s/relative/relevant/

Also, maybe just cite the pref wholesale: toolkit.telemetry.archive.enabled

::: toolkit/components/telemetry/docs/concepts/index.rst
@@ +4,5 @@
>  
> +There are common concepts used throughout Telemetry:
> +
> +* :doc:`pings <pings>` - the packets we use to submit data
> +* :doc:`sessions & subsessions <sessions>` - how we slice a users time in the browser

s/a users/users'/

::: toolkit/components/telemetry/docs/concepts/pings.rst
@@ +26,4 @@
>  
>  * :doc:`main <../data/main-ping>` - contains the information collected by Telemetry (Histograms, hang stacks, ...)
>  * :doc:`saved-session <../data/main-ping>` - has the same format as a main ping, but it contains the *"classic"* Telemetry payload with measurements covering the whole browser session. This is only a separate type to make storage of saved-session easier server-side. This is temporary and will be removed soon.
> +* :doc:`crash <../data/crash-ping>` - a ping that is captured and sent after Firefox crashed.

I think "after Firefox crashes" is slightly more natural than "after Firefox crashed" in this context.

::: toolkit/components/telemetry/docs/concepts/sessions.rst
@@ +21,5 @@
> +
> +Subsession splits
> +=================
> +
> +The first subsession starts when the browser starts. After that, we split the subsession for different reasons:

The flow of this document is stilted. The first section invites an immediate comparison of how subsessions as units of time differ from sessions as units of time. This section should be up one and titled "Subsessions". The above section should be below and titled "Subsession Data"
Attachment #8776653 - Flags: feedback?(chutten) → feedback+
(Assignee)

Updated

3 years ago
No longer blocks: 1276201
(Assignee)

Updated

3 years ago
Blocks: 1292530
(Assignee)

Comment 20

3 years ago
John, can you take a look at the demo docs and let me know if this looks helpful to you?

(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> I started a new build of the demo docs, this should be available soon here:
> http://gfritzsche-demo.readthedocs.io/en/latest/toolkit/components/telemetry/
> telemetry/concepts/index.html
> http://gfritzsche-demo.readthedocs.io/en/latest/toolkit/components/telemetry/
> telemetry/collection/index.html
Flags: needinfo?(jdorlus)
(Assignee)

Comment 21

3 years ago
Created attachment 8778290 [details] [diff] [review]
Part 2 - Flesh out docs/collection/

Addressed Chris points. Also discovered syntax highlighting in Sphinx, yay.
Attachment #8778290 - Flags: review?(alessio.placitelli)
Attachment #8778290 - Flags: feedback?(mreid)
(Assignee)

Updated

3 years ago
Attachment #8776652 - Attachment is obsolete: true
Attachment #8776652 - Flags: feedback?(mreid)
(Assignee)

Comment 22

3 years ago
Created attachment 8778294 [details] [diff] [review]
Part 3 - Flesh out docs/concepts/
Attachment #8778294 - Flags: review?(alessio.placitelli)
Attachment #8778294 - Flags: feedback?(mreid)
(Assignee)

Updated

3 years ago
Attachment #8776653 - Attachment is obsolete: true
Attachment #8776653 - Flags: feedback?(mreid)
(Assignee)

Comment 23

3 years ago
Created attachment 8778398 [details] [diff] [review]
Part 4 - Bonus - Add 'code-block' syntax highlighting where possible
Attachment #8778398 - Flags: review?(alessio.placitelli)
Comment on attachment 8778290 [details] [diff] [review]
Part 2 - Flesh out docs/collection/

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

This looks good, just a few observations in addition to the environment.rst one noted below:

- Should we still add the note about TelemetryLog not being supported by the tooling as stated in the google doc?
- Should we have all the bullet entries in the index page starting with a lower case letter/upper case? (nit, feel free to ignore)

::: toolkit/components/telemetry/docs/collection/environment.rst
@@ +1,5 @@
> +===========
> +Environment
> +===========
> +
> +Work in progress.

This file doesn't seem to be referenced by the other pages.

::: toolkit/components/telemetry/docs/collection/index.rst
@@ +13,5 @@
> +The current data collection possibilities include:
> +
> +* :doc:`scalars` allow recording of a single value (string, boolean, a number)
> +* :doc:`histograms` can efficiently record multiple data points
> +* ``environment`` data records information about the system and settings a session occurs in

This is not referencing the environment.rst file you've created. Was the file intentionally added to the patch but not referenced?
Attachment #8778290 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8778294 [details] [diff] [review]
Part 3 - Flesh out docs/concepts/

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

This looks really good, great image. I left some comments below, no need to address them in this patch.

::: toolkit/components/telemetry/docs/concepts/index.rst
@@ +5,5 @@
> +There are common concepts used throughout Telemetry:
> +
> +* :doc:`pings <pings>` - the packets we use to submit data
> +* :doc:`sessions & subsessions <sessions>` - how we slice a users' time in the browser
> +* *measurements* - how we collect data

nit: should we link this to "Data CollectioN"?

@@ +6,5 @@
> +
> +* :doc:`pings <pings>` - the packets we use to submit data
> +* :doc:`sessions & subsessions <sessions>` - how we slice a users' time in the browser
> +* *measurements* - how we collect data
> +* *opt-in* & *opt-out* - the different sets of data we collect

Should we file a bug about filling this part? Maybe we can just add the overview from the onboarding slides, but I feel like it should be documented.

::: toolkit/components/telemetry/docs/concepts/sessions.rst
@@ +20,5 @@
> +
> +* ``shutdown``, when the browser was cleanly shut down. To avoid delaying shutdown, we only save this ping to disk and send it at the next opportunity (typically the next browsing session).
> +* ``aborted-session``, when the browser crashed. While Firefox is active, we write the current ``main`` ping data to disk every 5 minutes. If the browser crashes, we find this data on disk on the next start and send it with this reason.
> +
> +.. image:: subsession_triggers.png

That's crystal clear, great job!

@@ +35,5 @@
> +
> +* Latency - Sending a ping with all the data of a subsession immediately after it ends means we get the data from installs faster. For ``main`` pings, we aim to send a ping at least daily by starting a new subsession at local midnight.
> +* Correlation - By starting new subsessions when fundamental settings change (i.e. changes to the *environment*), we can correlate a subsessions data better to those settings.
> +
> +

Maybe we could spend a few words on subsession chaining here. If useful, that doesn't need to happen in this bug.
Attachment #8778294 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8778398 [details] [diff] [review]
Part 4 - Bonus - Add 'code-block' syntax highlighting where possible

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

::: toolkit/components/telemetry/docs/data/heartbeat-ping.rst
@@ +23,5 @@
>        type: "heartbeat",
>        version: 4,
>        clientId: <UUID>,
> +      environment: { /* ... */ }
> +      // ... common ping data

Any reason why these changes are needed? I see you didn't do that in the other files and they seem to look ok as well (e.g. main ping)
Attachment #8778398 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8778290 [details] [diff] [review]
Part 2 - Flesh out docs/collection/

> +- `gzipServer <https://github.com/vdjeric/gzipServer>`_ - a Python script that can run locally and receives and saves...

We should fork this to a repo in the "mozilla" group.
Attachment #8778290 - Flags: feedback?(mreid) → feedback+
Comment on attachment 8778294 [details] [diff] [review]
Part 3 - Flesh out docs/concepts/

+To allow for cheaper lookup of archived pings, storage follows a specific naming scheme for both the directory and the ping file name: `<YYYY-MM>/<timestamp>.<UUID>.<type>.json`.

Aren't these named ".jsonlz4"?
Attachment #8778294 - Flags: feedback?(mreid) → feedback+
(Assignee)

Comment 29

3 years ago
(In reply to Alessio Placitelli [:Dexter] from comment #26)
> Comment on attachment 8778398 [details] [diff] [review]
> Part 4 - Bonus - Add 'code-block' syntax highlighting where possible
> 
> Review of attachment 8778398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/docs/data/heartbeat-ping.rst
> @@ +23,5 @@
> >        type: "heartbeat",
> >        version: 4,
> >        clientId: <UUID>,
> > +      environment: { /* ... */ }
> > +      // ... common ping data
> 
> Any reason why these changes are needed? I see you didn't do that in the
> other files and they seem to look ok as well (e.g. main ping)

They failed the syntax highlighter parser, the changes make them pass.
(Assignee)

Updated

3 years ago
Attachment #8775116 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8775116 - Flags: review+ → review?(alessio.placitelli)
Comment on attachment 8775116 [details] [diff] [review]
Part 1 - Restructure Telemetry client documentation

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

::: toolkit/components/telemetry/docs/data/index.rst
@@ +1,3 @@
> +==================
> +Data documentation
> +==================

nit: should we include a link to the schemas repo here? Or probably in the pings.rst file.
Attachment #8775116 - Flags: review?(alessio.placitelli) → review+

Comment 31

3 years ago
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/fx-team/rev/4cde9fc0a821
Part 1 - Restructure Telemetry client documentation. r=dexter, f=chutten
https://hg.mozilla.org/integration/fx-team/rev/5731b7d08ed6
Part 2 - Flesh out docs/collection/. r=dexter, f=chutten,mreid
https://hg.mozilla.org/integration/fx-team/rev/1ff3d8af70e8
Part 3 - Flesh out docs/concepts/. r=dexter, f=chutten,mreid
https://hg.mozilla.org/integration/fx-team/rev/9c17fe67ae98
Part 4 - Bonus - Add 'code-block' syntax highlighting where possible. r=dexter
(In reply to Georg Fritzsche [:gfritzsche] from comment #20)
> John, can you take a look at the demo docs and let me know if this looks
> helpful to you?
> 
> (In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> > I started a new build of the demo docs, this should be available soon here:
> > http://gfritzsche-demo.readthedocs.io/en/latest/toolkit/components/telemetry/
> > telemetry/concepts/index.html
> > http://gfritzsche-demo.readthedocs.io/en/latest/toolkit/components/telemetry/
> > telemetry/collection/index.html

I have looked at most of the docs. I see a few minute details that could prove helpful like linking to documents when we mention them: i.e "code documentation" in scalars.rst. I could add the changes myself or comment on the commit. Whichever would prove to be more beneficial.
Flags: needinfo?(jdorlus) → needinfo?(gfritzsche)
(Assignee)

Comment 33

3 years ago
Thanks John. This bug has landed now, so i would take future improvements to a new bug.
Can you file a new bug on this in Toolkit::Telemetry and list the suggestions?
If you can provide a patch there, that would be great!
Flags: needinfo?(gfritzsche)
(Assignee)

Updated

3 years ago
Flags: needinfo?(jdorlus)

Comment 34

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4cde9fc0a821
https://hg.mozilla.org/mozilla-central/rev/5731b7d08ed6
https://hg.mozilla.org/mozilla-central/rev/1ff3d8af70e8
https://hg.mozilla.org/mozilla-central/rev/9c17fe67ae98
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
I went through these documents from a user/developer standpoint. All looks well. I mean, there are small issues like punctuation and stuff but I didn't submit patches to add punctuation. Do we turn doc over to a tech writing team for that kind of stuff?
Status: RESOLVED → VERIFIED
Flags: needinfo?(jdorlus)
(Assignee)

Comment 36

3 years ago
Thanks John.
We don't have a specific tech writing team (outside of initiatives like MDN), this documentation is our responsibility.
If you feel like going over this and and submit a patch with accumulated small fixes, thats great too.
Otherwise we'll fix it as we touch it.
status-firefox49: affected → wontfix
You need to log in before you can comment on or make changes to this bug.