1277810 - Restructure Telemetry client documentation

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

(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

[Benjamin Smedberg]

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

Comment 2

[Georg Fritzsche [:gfritzsche]]

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.

Comment 3

[Georg Fritzsche [:gfritzsche]]

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/

Comment 4

[Georg Fritzsche [:gfritzsche]]

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

Comment 5

[Georg Fritzsche [:gfritzsche]]

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.

Comment 6

[Chris H-C :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

Comment 7

[Georg Fritzsche [:gfritzsche]]

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

Comment 8

[Georg Fritzsche [:gfritzsche]]

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

Comment 9

[Georg Fritzsche [:gfritzsche]]

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.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1 - Restructure Telemetry client documentation

Fine-tuned some TOC handling.

Comment 11

[Chris H-C :chutten]

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.

Comment 12

[Georg Fritzsche [:gfritzsche]]

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.

Comment 13

[Mark Reid [:mreid]]

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.

Comment 14

[Georg Fritzsche [:gfritzsche]]

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/

Comment 15

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2 - Flesh out docs/collection/

This sketches out the collection documentation. Its not aiming for perfection, we can fill this out more later.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 3 - Flesh out docs/concepts/

Ditto for the concepts documentation.

Comment 17

[Georg Fritzsche [:gfritzsche]]

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

Comment 18

[Chris H-C :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?

Comment 19

[Chris H-C :chutten]

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"

Comment 20

[Georg Fritzsche [:gfritzsche]]

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

Comment 21

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2 - Flesh out docs/collection/

Addressed Chris points. Also discovered syntax highlighting in Sphinx, yay.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 3 - Flesh out docs/concepts/

Comment 23

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 4 - Bonus - Add 'code-block' syntax highlighting where possible

Comment 24

[Alessio Placitelli [:Dexter]]

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?

Comment 25

[Alessio Placitelli [:Dexter]]

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.

Comment 26

[Alessio Placitelli [:Dexter]]

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)

Comment 27

[Mark Reid [:mreid]]

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.

Comment 28

[Mark Reid [:mreid]]

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"?

Comment 29

[Georg Fritzsche [:gfritzsche]]

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

Comment 30

[Alessio Placitelli [:Dexter]]

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.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
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

Comment 32

[John Dorlus [:Silne30]]

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

Comment 33

[Georg Fritzsche [:gfritzsche]]

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!

Comment 34

[Carsten Book [:Tomcat]]

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

Comment 35

[John Dorlus [:Silne30]]

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?

Comment 36

[Georg Fritzsche [:gfritzsche]]

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.