1301364 - Add short code samples to the Scalar docs

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

We should add short examples for plain & keyed, JS & C++ usage.

The doc file lives here: https://dxr.mozilla.org/mozilla-central/rev/ab70808cd4b6c6ad9a57a9f71cfa495fcea0aecd/toolkit/components/telemetry/docs/collection/scalars.rst

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1301364.patch

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8834318 [details] [diff] [review]
bug1301364.patch

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

This looks helpful.

Side-note: With harters on-going plans, we might move this kind of guidance/prose information elsewhere later, but that shouldn't block us now.

::: toolkit/components/telemetry/docs/collection/scalars.rst
@@ +161,5 @@
> +
> +.. code-block:: yaml
> +
> +    # The following section contains the demo scalars.
> +    demo.scalars:

I find it helpful to use examples that are from real work (or could be).
For some people that sticks much better.
Why not use e.g. some real UI work or other features here?

@@ +169,5 @@
> +        description: A boolean scalar to record the presence of a feature.
> +        expires: "60"
> +        kind: boolean
> +        notification_emails:
> +          - [e-mail]

Better use a placeholder email in case of copy/paste.

@@ +197,5 @@
> +Changing the demo scalars from privileged JavaScript code is straightforward:
> +
> +.. code-block:: js
> +
> +  // Set the scalar value to True: trying to use a non-boolean value doesn't throw

s/True/true/
"to True" doesn't really add information though.

@@ +199,5 @@
> +.. code-block:: js
> +
> +  // Set the scalar value to True: trying to use a non-boolean value doesn't throw
> +  // but rather prints a warning to the browser console
> +  Services.telemetry.scalarSet("demo.scalars.has_nice_feature", True);

Ditto.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1301364.patch

Thanks Georg. I think this addresses your comments.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8834421 [details] [diff] [review]
bug1301364.patch

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

::: toolkit/components/telemetry/docs/collection/scalars.rst
@@ +152,5 @@
>  
> +Adding a new probe
> +==================
> +Making a scalar measurement is a two step process that requires adding the probe definition
> +to the scalar registry and then calling the API at the measurement site.

Lets make this more clear by adding a two point list?
E.g.:
> ... is a two-step process:
> 1. add the probe definition to the scalar registry
> 2. record into the scalar using the API

@@ +220,5 @@
> +
> +    Telemetry::ScalarAdd(Telemetry::ScalarID::UI_DOWNLOAD_BUTTON_ACTIVATED,
> +                         NS_LITERAL_STRING("touchscreen"), 1);
> +
> +The ``ScalarID`` enum is automatically generated by the build process, with an example being available `here <https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/toolkit/components/telemetry/TelemetryScalarEnums.h>`_ .

This link is prone to become outdated. Maybe just mention the filename or use a "path:TelemetryScalarEnums.h" search for the link?

@@ +222,5 @@
> +                         NS_LITERAL_STRING("touchscreen"), 1);
> +
> +The ``ScalarID`` enum is automatically generated by the build process, with an example being available `here <https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/toolkit/components/telemetry/TelemetryScalarEnums.h>`_ .
> +
> +The Scalars C++ API also has `test coverage <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/gtest/TestScalars.cpp>`_ that can be used for other examples.

Nit: "Other examples can be found in the test coverage ... for the scalars C++ API."

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: bug1301364.patch

Comment 6

[Alessio Placitelli [:Dexter]]

This is only docs, doesn't need a try push :)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c36e60446a97
Add short code samples to the Scalar docs. r=gfritzsche

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c36e60446a97

Comment 9

[Gerry Chang [:gchang]]

Too late for 51. Mark 51 won't fix.