All previous event telemetry lost when sending a new event

RESOLVED FIXED in Firefox 62

Status

()

defect
P1
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: miker, Assigned: janerik)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

STR:

1. Add this to Events.yaml as the last child of devtools.main:
  edit_html:
    objects: ["inspector"]
    bug_numbers: [1463080]
    notification_emails: ["dev-developer-tools@lists.mozilla.org", "hkirschner@mozilla.com"]
    record_in_processes: ["main"]
    description: User is editing HTML via the context menu item in the markup view.
    release_channel_collection: opt-out
    expiry_version: never
    extra_keys:
      made_changes: Indicates whether changes were made.
      time_open: The amount of time in ms that the HTML editor was open.

2. ./mach build
3. ./mach run
3. Open the devtools and close them to produce some event telemetry.
4. Go to about:telemetry.
5. Click "Events" and some pings will be present.
5. Open the browser console.
6. Enter the following in a privileged scratchpad:
   Services.telemetry.recordEvent("devtools.main", "edit_html", "inspector", null, {
     "made_changes": "true", 
     "time_open": "1234"
   });
7. Click run.

Refresh about:telemetry and you will see that all previous event telemetry has been disappeared and only the edit_html ping exists.
Summary: Changes to Events.yaml not available to browser mochitests when using artifact builds → All previous event telemetry ;lost wehen sending a new ping
Summary: All previous event telemetry ;lost wehen sending a new ping → All previous event telemetry lost wehen sending a new ping
Summary: All previous event telemetry lost wehen sending a new ping → All previous event telemetry lost when sending a new ping
I have checked the archived data and the data is not there either.
Summary: All previous event telemetry lost when sending a new ping → All previous event telemetry lost when sending a new event
This is an artifact build.
(Assignee)

Updated

11 months ago
Assignee: nobody → jrediger
You are right, this works fine with a full build.
Comment hidden (mozreview-request)
Do the patches here conflict with bug 1460595?
Given that bugs complexity, should we land bug 1460595 first?

Comment 7

11 months ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> Do the patches here conflict with bug 1460595?
> Given that bugs complexity, should we land bug 1460595 first?

Yes, but they simplify things. I'm not too worried about resolving it.

That being said, I haven't reviewed them yet, so let's not get ahead of ourselves about when they'll be landing :D

Comment 8

11 months ago
mozreview-review
Comment on attachment 8980235 [details]
Bug 1463742 - Remove separate dynamic-builtin event storage.

https://reviewboard.mozilla.org/r/246390/#review252992

Looks fine to me.

It took me a while to page in the history of why we had multiple storage. It hearkens back to the Scalar implementation of dynamic builtins which required the secondary storage because of its use of IDs. Since the keys to event storage are "category#method#object", this concern doesn't apply and so it shouldn't matter.

That being said, I'm adding Alessio to this review so that he also gets the fun of trying to remember why there are two event storages instead of one.
Attachment #8980235 - Flags: review?(chutten) → review+

Updated

11 months ago
Attachment #8980235 - Flags: review?(alessio.placitelli)

Comment 9

11 months ago
mozreview-review
Comment on attachment 8980236 [details]
Bug 1463742 - Test that dynamic builtin events are correctly recorded in child processes.

https://reviewboard.mozilla.org/r/246392/#review252994
Attachment #8980236 - Flags: review?(chutten) → review+

Comment 10

11 months ago
mozreview-review
Comment on attachment 8980235 [details]
Bug 1463742 - Remove separate dynamic-builtin event storage.

https://reviewboard.mozilla.org/r/246390/#review253238

Gosh, now I remember.  Thanks Chris, double storage was cargo culted from Scalars but doesn't seem to be needed due to the way event ids work. Good catch Jan-Erik!

::: commit-message-a4ad0:4
(Diff revision 1)
> +Bug 1463742 - Remove separate dynamic-builtin event storage. r?chutten
> +
> +Using a separate storage just for dynamic-builtin events is unnecessary
> +and leads to additional problems along the way.

Let's add the context from Chris (comment 8) to explain why double storage is not needed for events.
Attachment #8980235 - Flags: review?(alessio.placitelli) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 13

11 months ago
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fce8e1904b18
Remove separate dynamic-builtin event storage. r=chutten,Dexter
https://hg.mozilla.org/integration/autoland/rev/ea1bc9529569
Test that dynamic builtin events are correctly recorded in child processes. r=chutten

Comment 14

11 months ago
Backed out 2 changesets (bug 1463742) for XPCShell failures on toolkit/components/telemetry/tests/unit/test_TelemetryChildEvents_buildFaster.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=180562889&repo=autoland&lineNumber=1960

 TEST-START | toolkit/components/telemetry/tests/unit/test_TelemetryChildEvents_buildFaster.js
[task 2018-05-28T14:07:26.450Z] 14:07:26  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryChildEvents_buildFaster.js | xpcshell return code: 139
[task 2018-05-28T14:07:26.451Z] 14:07:26     INFO -  TEST-INFO took 8932ms
[task 2018-05-28T14:07:26.451Z] 14:07:26     INFO -  >>>>>>>
[task 2018-05-28T14:07:26.452Z] 14:07:26     INFO -  toolkit/components/telemetry/tests/unit/test_TelemetryChildEvents_buildFaster.js | xpcw: cd /sdcard/tests/xpc/toolkit/components/telemetry/tests/unit
[task 2018-05-28T14:07:26.453Z] 14:07:26     INFO -  toolkit/components/telemetry/tests/unit/test_TelemetryChildEvents_buildFaster.js | xpcw: xpcshell -r /sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/sdcard/tests/xpc/m"; -f /sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/sdcard/tests/xpc/toolkit/components/telemetry/tests/unit/head.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_TelemetryChildEvents_buildFaster.js"]; -e const _TEST_NAME = "toolkit/components/telemetry/tests/unit/test_TelemetryChildEvents_buildFaster.js" -e _execute_test(); quit(0);
[task 2018-05-28T14:07:26.453Z] 14:07:26     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-05-28T14:07:26.454Z] 14:07:26     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-05-28T14:07:26.454Z] 14:07:26     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-05-28T14:07:26.454Z] 14:07:26     INFO -  running event loop
[task 2018-05-28T14:07:26.455Z] 14:07:26     INFO -  toolkit/components/telemetry/tests/unit/test_TelemetryChildEvents_buildFaster.js | Starting test_setup

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ea1bc9529569b8845f91a2fb355db807f3610741&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Backout:
https://hg.mozilla.org/integration/autoland/rev/41a876c31861b1a60da3049073c420c02a18d919
Flags: needinfo?(jrediger)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Flags: needinfo?(jrediger)

Comment 16

11 months ago
mozreview-review
Comment on attachment 8980236 [details]
Bug 1463742 - Test that dynamic builtin events are correctly recorded in child processes.

https://reviewboard.mozilla.org/r/246392/#review253468
Attachment #8980236 - Flags: review?(alessio.placitelli) → review+

Comment 17

11 months ago
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1f2e10753a7
Remove separate dynamic-builtin event storage. r=chutten,Dexter
https://hg.mozilla.org/integration/autoland/rev/55ac468514e8
Test that dynamic builtin events are correctly recorded in child processes. r=chutten,Dexter

Comment 18

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e1f2e10753a7
https://hg.mozilla.org/mozilla-central/rev/55ac468514e8
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.