Closed Bug 1463742 Opened 6 years ago Closed 6 years ago

All previous event telemetry lost when sending a new event

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla62

Tracking Flags:

Tracking

Status

firefox62

---

fixed

People

(Reporter: miker, Assigned: janerik)

Details

Attachments

(2 files)

Bug 1463742 - Remove separate dynamic-builtin event storage. 6 years ago Jan-Erik Rediger [:janerik] 59 bytes, text/x-review-board-request	chutten : review+	Details
Bug 1463742 - Test that dynamic builtin events are correctly recorded in child processes. 6 years ago Jan-Erik Rediger [:janerik] 59 bytes, text/x-review-board-request	Dexter : review+	Details

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

Reporter

Description

•

6 years ago

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.

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

Reporter

Updated

•

6 years ago

Summary: Changes to Events.yaml not available to browser mochitests when using artifact builds → All previous event telemetry ;lost wehen sending a new ping

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

Reporter

Updated

•

6 years ago

Summary: All previous event telemetry ;lost wehen sending a new ping → All previous event telemetry lost wehen sending a new ping

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

Reporter

Updated

•

6 years ago

Summary: All previous event telemetry lost wehen sending a new ping → All previous event telemetry lost when sending a new ping

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

Reporter

Comment 1

•

6 years ago

I have checked the archived data and the data is not there either.

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

Reporter

Updated

•

6 years ago

Summary: All previous event telemetry lost when sending a new ping → All previous event telemetry lost when sending a new event

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

Reporter

Comment 2

•

6 years ago

This is an artifact build.

Jan-Erik Rediger [:janerik]

Assignee

Updated

•

6 years ago

Assignee: nobody → jrediger

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

Reporter

Comment 3

•

6 years ago

You are right, this works fine with a full build.

Comment hidden (mozreview-request)

Using a separate storage just for dynamic-builtin events is unnecessary
and leads to additional problems along the way.
One of them is the snapshotter failing to properly accumulate from both
storages into a single array.
In addition, even if those are merged, an additional sort step would be
necessary to keep events ordered by time of occurence. And even then it
can't guarantee exact order for events from different storages but same
millisecond-time.

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

Comment hidden (mozreview-request)

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

Georg Fritzsche [:gfritzsche]

Comment 6

•

6 years ago

Do the patches here conflict with bug 1460595?
Given that bugs complexity, should we land bug 1460595 first?

Chris H-C :chutten

Comment 7

•

6 years 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

Chris H-C :chutten

Comment 8

•

6 years 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+

Chris H-C :chutten

Updated

•

6 years ago

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

Chris H-C :chutten

Comment 9

•

6 years 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+

Alessio Placitelli [:Dexter]

Comment 10

•

6 years 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 on attachment 8980235 [details]
Bug 1463742 - Remove separate dynamic-builtin event storage.

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

Comment hidden (mozreview-request)

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

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

Jan-Erik Rediger [:janerik]

Assignee

Updated

•

6 years ago

Keywords: checkin-needed

Jan-Erik Rediger [:janerik]

Assignee

Updated

•

6 years ago

Keywords: checkin-needed

Pulsebot

Comment 13

•

6 years 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

Dorel Luca [:dluca]

Comment 14

•

6 years 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)

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

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

Jan-Erik Rediger [:janerik]

Assignee

Updated

•

6 years ago

Flags: needinfo?(jrediger)

Alessio Placitelli [:Dexter]

Comment 16

•

6 years 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+

Pulsebot

Comment 17

•

6 years 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

Noemi Erli[:noemi_erli]

Comment 18

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/e1f2e10753a7
https://hg.mozilla.org/mozilla-central/rev/55ac468514e8

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox62: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla62

You need to log in before you can comment on or make changes to this bug.