Track toolbox session id in event telemetry probes

RESOLVED FIXED in Firefox 62

Status

enhancement
P2
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: Harald, Assigned: miker)

Tracking

(Blocks 1 bug)

unspecified
Firefox 63
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox62 fixed, firefox63 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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

Updated

a year ago
Flags: needinfo?(mratcliffe)
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.
Flags: needinfo?(mratcliffe)
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All
Reporter

Comment 2

a year ago
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.
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
(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.
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

Updated

11 months ago
Product: Firefox → DevTools
Reporter

Comment 6

11 months ago
Per Tim Smith, the session id would then just be the toolbox opening time. That does simplify things. Does that work, Mike?
Flags: needinfo?(mratcliffe)
Yep... just getting the current UNIX time is simple so let's do that.
Flags: needinfo?(mratcliffe)
Reporter

Comment 8

11 months ago
This is making the M1 event telemetry impractical to analyze. We should land this soon, validate and uplift to Beta.
Posted file data peer reveiw request.txt (obsolete) —
Attachment #8987009 - Flags: review?(francois)
@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.
Flags: needinfo?(francois)
The session_id will contain a UNIX timestamp e.g. 1396381378123
Assignee: nobody → mratcliffe
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.
Flags: needinfo?(francois) → needinfo?(chutten)
Attachment #8987009 - Flags: review?(francois)

Comment 14

11 months ago
mozreview-review
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
Attachment #8987054 - Flags: review?(ystartsev) → review+
Attachment #8987009 - Flags: review?(chutten)

Comment 15

11 months ago
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.)
Flags: needinfo?(chutten) → needinfo?(mratcliffe)
Attachment #8987009 - Flags: review?(chutten) → review-
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
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"]."]
Flags: needinfo?(mratcliffe)
Comment hidden (mozreview-request)
They were missed because changes to existing events in Events.yaml still need a full build.
Posted file data-review.txt
Attachment #8987009 - Attachment is obsolete: true
Attachment #8987859 - Flags: review?(chutten)
Of course, we now use msSinceProcessStart() as the session ID as requested.

Comment 22

11 months ago
(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

11 months ago
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+
Attachment #8987859 - Flags: review?(chutten) → review+
Comment hidden (mozreview-request)
(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

11 months ago
:janerik, do we support the use case from Comment#19 ?
Flags: needinfo?(jrediger)

Comment 27

11 months ago
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e52c11b8f8ec
Track toolbox session id in event telemetry probes r=yulia

Comment 29

11 months ago
Backout by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa8226ea0c48
Backed out 1 changesets for devtools perfails browser_exit_button.js CLOSED TREE
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
Flags: needinfo?(jrediger)
Comment hidden (mozreview-request)
Assignee

Comment 32

11 months ago
mozreview-review
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

11 months ago
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57c6388be227
Track toolbox session id in event telemetry probes r=yulia
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
Flags: needinfo?(mratcliffe)
Comment hidden (mozreview-request)
Okay, now try is completely green.
Flags: needinfo?(mratcliffe)

Comment 37

11 months ago
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69c641de1fa6
Track toolbox session id in event telemetry probes r=yulia

Comment 38

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/69c641de1fa6
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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
Attachment #8987054 - Flags: approval-mozilla-beta?
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.
Attachment #8987054 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.