Closed Bug 1299772 Opened 8 years ago Closed 7 years ago

Document stack capturing API in the in-tree docs

Categories

(Toolkit :: Telemetry, defect, P3)

Product:
Toolkit
Component:
Telemetry
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: gfritzsche, Assigned: yarik.sheptykin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client])

Attachments

(2 files)

Bug1299772: Document stack capturing API.
59 bytes, text/x-review-board-request
gfritzsche
: review+
Details
Bug 1299772: Document stack capturing API.
59 bytes, text/x-review-board-request
gfritzsche
: review+
Details 
After bug 1225851, we should document its C++ API and the JS API in `toolkit/components/telemetry/docs/collection/`.
This can follow the style of documentation that we did for scalars:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/docs/collection/scalars.rst

Here you can see the generated documentation: 
https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/index.html

Locally you can build it using `mach doc` and will find it in <objdir>/docs/html/...
Hi Gerg!

Please take a look at my first draft of the docs. I hope it is moving into the right direction.
Assignee: nobody → yarik.sheptykin
Comment on attachment 8831792 [details]
Bug1299772: Document stack capturing API.

https://reviewboard.mozilla.org/r/108330/#review109578

This looks pretty good already.

Besides the comments below, please also reference the new page from main-ping.rst. See the UITelemetry reference for an example:
https://dxr.mozilla.org/mozilla-central/rev/1fe66bd0efba89df59d2046e8c91418eb5ae10b8/toolkit/components/telemetry/docs/data/main-ping.rst#167

The reference used there is created here:
https://dxr.mozilla.org/mozilla-central/rev/1fe66bd0efba89df59d2046e8c91418eb5ae10b8/browser/docs/UITelemetry.rst#1

::: toolkit/components/telemetry/docs/collection/stacks.rst:2
(Diff revision 1)
> +======
> +Stacks

Just "Stacks" is a bit generic for the name.
I'd propose we go with "Stack capture" & stack-capture.rst

::: toolkit/components/telemetry/docs/collection/stacks.rst:10
(Diff revision 1)
> +While studying behavior of Firefox in the wild it is sometimes useful to inspect
> +call stacks without causing the browser to crash. Historically we could only
> +obtain stacks for inspection from crash reports. Now stacks can be captured on
> +demand and annotated with a unique key for further inspection.
> +
> +Capturing stacks is only supported on official builds with "--enable-profiling"

Instead of using quotes for "--enable-profiling", can you format this as include code using backticks?

::: toolkit/components/telemetry/docs/collection/stacks.rst:24
(Diff revision 1)
> +
> +The API
> +=======
> +Capturing stacks is available either via the `nsITelemetry interface <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/nsITelemetry.idl>`_
> +or the `C++ API <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.h>`_.
> +

You should add a note that:
- the API is currently not thread-safe
- that recording in content processes is not supported yet

::: toolkit/components/telemetry/docs/collection/stacks.rst:32
(Diff revision 1)
> +Privileged JavaScript code can capture stacks using the following function:
> +
> +.. code-block:: js
> +
> +  Services.telemetry.captureStack(aKey);
> +

You should:
- add one sentence o what the function does
- document parameters, i.e. that `aKey` is a string and it is good for
Attachment #8831792 - Flags: review?(gfritzsche)
We also need to document the internal sanity limits:
- maximum stack depth
- maximum number of different keys/stacks can be captured
Status: NEW → ASSIGNED
Comment on attachment 8831792 [details]
Bug1299772: Document stack capturing API.

https://reviewboard.mozilla.org/r/108330/#review111064

This looks good with the below fixed.

::: toolkit/components/telemetry/docs/collection/stack-capture.rst:13
(Diff revision 2)
> +demand and annotated with a unique key for further inspection.
> +
> +Capturing stacks is only supported on official builds with ``--enable-profiling``
> +switch enabled, such as Nightly builds, for example. The feature is available on
> +Windows, Linux and OSX. Capturing stacks is only available in the extended set
> +of measures.

"... available with extended Telemetry enabled."

::: toolkit/components/telemetry/docs/collection/stack-capture.rst:67
(Diff revision 2)
> +
> +To prevent abuses, the content of a key is limited to 50 characters in length.
> +Additionally, keys may only contain alpha-numeric characters or ``-``.
> +
> +Both the depth of the captured stacks and the total number of keys in the
> +dictionary to  are limited to ``50``.

"to  are"?

::: toolkit/components/telemetry/docs/data/main-ping.rst:292
(Diff revision 2)
>        ... other threads ...
>       ]
>  
>  capturedStacks
>  --------------
> -Contains information about stacks captured on demand via Telemetry API. This is similar to `chromeHangs`, but only stacks captured on the main thread of the parent process are reported. It reports precise C++ stacks are reported and is only available on Windows, either in Firefox Nightly or in builds using "--enable-profiling" switch.
> +Contains information about stacks captured on demand via Telemetry API. For more see :doc:`stack capture <../collection/stack-capture>`.

"For details see ..."
or "For more information see ..."

::: toolkit/components/telemetry/docs/data/main-ping.rst:294
(Diff revision 2)
>  
>  capturedStacks
>  --------------
> -Contains information about stacks captured on demand via Telemetry API. This is similar to `chromeHangs`, but only stacks captured on the main thread of the parent process are reported. It reports precise C++ stacks are reported and is only available on Windows, either in Firefox Nightly or in builds using "--enable-profiling" switch.
> +Contains information about stacks captured on demand via Telemetry API. For more see :doc:`stack capture <../collection/stack-capture>`.
> +
> +This is similar to `chromeHangs`, but only stacks captured on the main thread of the parent process are reported. It reports precise C++ stacks are reported and is only available on Windows, either in Firefox Nightly or in builds using ``--enable-profiling`` switch.

"It reports [...] are reported"?
Attachment #8831792 - Flags: review?(gfritzsche) → review+
Comment on attachment 8833999 [details]
Bug 1299772: Document stack capturing API.

https://reviewboard.mozilla.org/r/110108/#review111086

::: toolkit/components/telemetry/docs/collection/index.rst:35
(Diff revision 1)
>     scalars
>     histograms
>     events
>     measuring-time
>     custom-pings
> -   *
> +   stack-capture

Oh, I think I should have kept `*` where it was, right?
(In reply to Iaroslav Sheptykin from comment #9)
> Comment on attachment 8833999 [details]
> Bug1299772: Document stack capturing API.
> 
> https://reviewboard.mozilla.org/r/110108/#review111086
> 
> ::: toolkit/components/telemetry/docs/collection/index.rst:35
> (Diff revision 1)
> >     scalars
> >     histograms
> >     events
> >     measuring-time
> >     custom-pings
> > -   *
> > +   stack-capture
> 
> Oh, I think I should have kept `*` where it was, right?

Yes please. That is the place holder for "documents in this directory that were not referenced yet".
Comment on attachment 8833999 [details]
Bug 1299772: Document stack capturing API.

https://reviewboard.mozilla.org/r/110110/#review111490

Thanks!
Attachment #8833999 - Flags: review?(gfritzsche) → review+
Do we have try builds for docs? Otherwise we can land this one, right?
Flags: needinfo?(gfritzsche)
I tried to autoland, see MozReview for the resulting error message.
Flags: needinfo?(gfritzsche)
Priority: P5 → P3
Alright.

The error was:

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 3 changes to 3 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 513b91ae0599 needs "Bug N" or "No bug" in the commit message.
remote: Iaroslav (yarik) Sheptykin <yarik.sheptykin@googlemail.com>
remote: Bug1299772: Document stack capturing API. r=gfritzsche
remote: 
remote: MozReview-Commit-ID: G2IwndgznaX
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote


Apparently I need to separate `Bug` and bug number with a space. Updated the review. Can we try again?
Flags: needinfo?(gfritzsche)
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/autoland/rev/7c28fb339a2a
Document stack capturing API. r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/7c28fb339a2a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: needinfo?(gfritzsche)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: