Closed Bug 1301364 Opened 6 years ago Closed 5 years ago

Add short code samples to the Scalar docs

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox54 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 2 obsolete files)

Blocks: 1276201
Points: --- → 1
Priority: -- → P3
Whiteboard: [measurement:client]
Priority: P3 → P1
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attached patch bug1301364.patch (obsolete) — Splinter Review
Attachment #8834318 - Flags: review?(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:
> +          - telemetry-client-dev@mozilla.com

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.
Attachment #8834318 - Flags: review?(gfritzsche) → feedback+
Attached patch bug1301364.patch (obsolete) — Splinter Review
Thanks Georg. I think this addresses your comments.
Attachment #8834318 - Attachment is obsolete: true
Attachment #8834421 - Flags: review?(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."
Attachment #8834421 - Flags: review?(gfritzsche) → review+
Attached patch bug1301364.patchSplinter Review
Attachment #8834421 - Attachment is obsolete: true
Attachment #8834432 - Flags: review+
This is only docs, doesn't need a try push :)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c36e60446a97
Add short code samples to the Scalar docs. r=gfritzsche
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c36e60446a97
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Too late for 51. Mark 51 won't fix.
You need to log in before you can comment on or make changes to this bug.