1466880 - Track toolbox session id in event telemetry probes

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[:Harald Kirschner :digitarald]

Key should be `toolbox_session`.

Amplitude has an existing session field that we should use for analyzing the data.

Events like RDM that don't require toolbox to be open should not have a session key when toolbox is not open.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

So it looks like we need a session ID per toolbox instance... the window id would probably be best here as we could get that without needing a reference to the toolbox. This may mean that all our tools need access to an instance of the toolbox.

Comment 2

[:Harald Kirschner :digitarald]

Mike, would window id be fresh after closing and re-opening the toolbox? Even if not, this would still help to create distict window specific tracking.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

We have a UUID module[1], perhaps we can use this to generate a reliably unique ID that we store on the toolbox instance?

[1]: https://searchfox.org/mozilla-central/source/devtools/shared/generate-uuid.js

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> We have a UUID module[1], perhaps we can use this to generate a reliably
> unique ID that we store on the toolbox instance?
> 
> [1]:
> https://searchfox.org/mozilla-central/source/devtools/shared/generate-uuid.js

Yep, that is going to be the best choice.

Comment 5

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

It looks like Amplitude event telemetry expects the session_id to be the milliseconds-since-epoch timestamp of the beginning of the session: https://amplitude.zendesk.com/hc/en-us/articles/204771828#optional%C2%A0keys

It uses this value directly to compute session length: https://amplitude.zendesk.com/hc/en-us/articles/115002323627-Tracking-Sessions#session-length

Comment 6

[:Harald Kirschner :digitarald]

Per Tim Smith, the session id would then just be the toolbox opening time. That does simplify things. Does that work, Mike?

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Yep... just getting the current UNIX time is simple so let's do that.

Comment 8

[:Harald Kirschner :digitarald]

This is making the M1 event telemetry impractical to analyze. We should land this soon, validate and uplift to Beta.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: data peer reveiw request.txt

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

@francois So the issue is that we need a toolbox session ID because more than one toolbox may be open at any one time. This will involve adding an extra_key to all of the probes under devtools.main in Events.yaml and also adding them to all of the data requests you have already approved under the meta bug 1456983.

Rather than create fifty something review requests I have attached this text to this bug and hopefully you can just r+ this.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The session_id will contain a UNIX timestamp e.g. 1396381378123

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1466880 - Track toolbox session id in event telemetry probes

Review commit: https://reviewboard.mozilla.org/r/252308/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252308/

Comment 13

[François Marier [:francois]]

Chris, would you have time to take care of this review?

I'm not personally going to have time to take a look at it until July.

Comment 14

[Yulia Startsev [:yulia] OOO until July 2026]

Comment on attachment 8987054 [details]
Bug 1466880 - Track toolbox session id in event telemetry probes

https://reviewboard.mozilla.org/r/252308/#review259126

Looks good to me, can be merged with a passing try run

Comment 15

[Chris H-C :chutten]

Comment on attachment 8987009 [details]
data peer reveiw request.txt

The "collection" here is the toolbox session id, so the Data Collection Review form need only be filled out once. I'll be happy to review the request.

While I'm here... Near as I can tell this session identifier is a millisecond-resolution timestamp from the client clock of the first interaction with the toolbox. The comment says Amplitude requires this? Will Amplitude care if the session timestamp thingy doesn't fall within the overall interaction time scale from the user? (presently the absolute timestamps used to order telemetry events are based on session start times which are rounded to the nearest hour. This will soon move to a different timestamp rounded to the nearest minute.)

Comment 16

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

chutten's question about timestamps led me to confer with Sunah about this and I think I misspecified the necessary behavior and the implementation here needs to change; sorry!

Amplitude's docs say they measure session length by subtracting the session_id from the latest event timestamp.

Although Amplitude's docs suggest specifying both the session_id and event timestamp as milliseconds-since-epoch, the Firefox implementation of event telemetry uses "milliseconds since the browser session began" instead. The event timestamps sent to Amplitude use this value directly.

The important thing is that the session_id should be directly comparable to the event timestamps, so the session_id should also be specified as milliseconds-since-session-start.

Sunah pointed to [1] as the code that returns the timestamps for events.

[1] https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryCommon.cpp#96

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Which is exactly why we have data reviews. Thanks for the feedback!

Also, as I side note a few events were missed:

Invalid extra key for event ["devtools.main", "activate", "responsive_design"]."]
Invalid extra key for event ["devtools.main", "activate", "split_console"]."]
Invalid extra key for event ["devtools.main", "close", "tools"]."]
Invalid extra key for event ["devtools.main", "deactivate", "responsive_design"]."]
Invalid extra key for event ["devtools.main", "enter", "inspector"]."]
Invalid extra key for event ["devtools.main", "exit", "inspector"]."]

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8987054 [details]
Bug 1466880 - Track toolbox session id in event telemetry probes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252308/diff/1-2/

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

They were missed because changes to existing events in Events.yaml still need a full build.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: data-review.txt

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Of course, we now use msSinceProcessStart() as the session ID as requested.

Comment 22

[Chris H-C :chutten]

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #19)
> They were missed because changes to existing events in Events.yaml still
> need a full build.

Didn't we fix that in bug 1448945 or one of its friends? ...I thought we had, but maybe we missed a case...

Comment 23

[Chris H-C :chutten]

Comment on attachment 8987859 [details]
data-review.txt

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Standard Telemetry mechanisms apply.

    Is there a control mechanism that allows the user to turn the data collection on and off? 

Standard Telemetry mechanisms apply.

    If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, Harald Kirschner.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? **

Category 2.

    Is the data collection request for default-on or default-off?

Default on.

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No. It is a permanent collection.

---
Result: datareview+

Comment 24

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8987054 [details]
Bug 1466880 - Track toolbox session id in event telemetry probes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252308/diff/2-3/

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Chris H-C :chutten from comment #22)
> (In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from
> comment #19)
> > They were missed because changes to existing events in Events.yaml still
> > need a full build.
> 
> Didn't we fix that in bug 1448945 or one of its friends? ...I thought we
> had, but maybe we missed a case...

I thought so but it obviously didn't work when I pushed this to try... most of the time I am still having to do full builds locally too.

Comment 26

[Chris H-C :chutten]

:janerik, do we support the use case from Comment#19 ?

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e52c11b8f8ec
Track toolbox session id in event telemetry probes r=yulia

Comment 28

[Jan-Erik Rediger [:janerik]]

It ... should work[1]. I'll keep the ni? and will double-check tomorrow.

[1]: https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js#241-289

Comment 29

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa8226ea0c48
Backed out 1 changesets for devtools perfails browser_exit_button.js CLOSED TREE

Comment 30

[Jan-Erik Rediger [:janerik]]

I double-checked build-faster events locally and modified events are picked up and correctly recognized. 
The above patch does record the data after a "./mach build faster" (I first added the code patch, ran a full build, then patched Events.yaml and ran "./mach build faster"). 
EventArtifactDefinitions.json in the build directory contains the correct extra keys. Manually executing the following code (when executed through devtools on about:telemetry) contains the expected event:

    Telemetry.recordEvent("devtools.main", "open", "tools", "zebra", {session_id: "test"});
    Telemetry.snapshotEvents(0, false).parent

Comment 31

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8987054 [details]
Bug 1466880 - Track toolbox session id in event telemetry probes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252308/diff/3-4/

Comment 32

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8987054 [details]
Bug 1466880 - Track toolbox session id in event telemetry probes

https://reviewboard.mozilla.org/r/252308/#review260694

Found the memory leak... we were calling `TargetFactory.forTab(tab)` but not destroying it before the browser closed. This would only have been a problem for the responsive design tool when opened outside the toolbox.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57c6388be227
Track toolbox session id in event telemetry probes r=yulia

Comment 34

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 57c6388be227 (bug 1466880) for devtools failures at devtools/client/responsive.html/test/browser/browser_telemetry_activate_rdm.js

Backout: https://hg.mozilla.org/integration/autoland/rev/b74ccfcd99d13dd4c68c2f9acad6ab03057f0189

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=57c6388be22786088ac9b35cf2e3c75bbf4c27e3&selectedJob=185611458

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=185611458&repo=autoland&lineNumber=14894

[task 2018-06-29T16:50:58.788Z]     INFO - TEST-PASS | devtools/client/responsive.html/test/browser/browser_telemetry_activate_rdm.js | RDM closed synchronously - 
[task 2018-06-29T16:50:58.797Z]     INFO - RDM's debugger client is now closed
[task 2018-06-29T16:50:58.866Z]     INFO - Responsive design mode closed
[task 2018-06-29T16:50:58.870Z]     INFO - TEST-INFO | started process screentopng
[task 2018-06-29T16:50:59.794Z]     INFO - TEST-INFO | screentopng: exit 0
[task 2018-06-29T16:50:59.794Z]     INFO - TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_telemetry_activate_rdm.js | Uncaught exception - at resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox-hosts.js:51 - TypeError: this.hostTab is null
[task 2018-06-29T16:50:59.795Z]     INFO - Stack trace:
[task 2018-06-29T16:50:59.795Z]     INFO - create@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox-hosts.js:51:5
[task 2018-06-29T16:50:59.797Z]     INFO - async*create@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox-host-manager.js:61:11
[task 2018-06-29T16:50:59.802Z]     INFO - async*createToolbox@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/devtools.js:540:27
[task 2018-06-29T16:50:59.802Z]     INFO - async*showToolbox@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/devtools.js:474:30
[task 2018-06-29T16:50:59.802Z]     INFO - async*@chrome://mochitests/content/browser/devtools/client/responsive.html/test/browser/browser_telemetry_activate_rdm.js:63:9
[task 2018-06-29T16:50:59.803Z]     INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1098:34
[task 2018-06-29T16:50:59.803Z]     INFO - async*Tester_execTest@chrome://mochikit/content/browser-test.js:1089:16
[task 2018-06-29T16:50:59.803Z]     INFO - nextTest/<@chrome://mochikit/content/browser-test.js:991:9
[task 2018-06-29T16:50:59.804Z]     INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2018-06-29T16:50:59.805Z]     INFO - Leaving test bound 
[task 2018-06-29T16:50:59.805Z]     INFO - Removing tab.
[task 2018-06-29T16:50:59.805Z]     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2018-06-29T16:50:59.806Z]     INFO - Got event: 'TabClose' on [object XULElement].
[task 2018-06-29T16:50:59.807Z]     INFO - Tab removed and finished closing
[task 2018-06-29T16:50:59.807Z]     INFO - GECKO(2585) | MEMORY STAT | vsize 2634MB | residentFast 510MB | heapAllocated 198MB

Comment 35

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8987054 [details]
Bug 1466880 - Track toolbox session id in event telemetry probes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252308/diff/4-5/

Comment 36

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Okay, now try is completely green.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69c641de1fa6
Track toolbox session id in event telemetry probes r=yulia

Comment 38

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/69c641de1fa6

Comment 39

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8987054 [details]
Bug 1466880 - Track toolbox session id in event telemetry probes

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: We won't be able to make sense of event telemetry on the Beta channel so product quality will suffer.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It is a very simple patch.
[String changes made/needed]: None

Comment 40

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8987054 [details]
Bug 1466880 - Track toolbox session id in event telemetry probes

Has data review, verified in Nightly, and we are still in early beta since it's a long beta cycle for 62. Seems ok for uplift for beta 7.

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/13622431fb71