Closed
Bug 1408975
Opened 7 years ago
Closed 7 years ago
Allow to register new addon events for an existing category
Categories
(Toolkit :: Telemetry, enhancement, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: gfritzsche)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
Per this thread:
https://mail.mozilla.org/pipermail/fx-data-dev/2017-October/000078.html
When an add-on updates, it may want to register additional events.
Currently we don't support this:
https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/toolkit/components/telemetry/TelemetryEvent.cpp#533
https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/toolkit/components/telemetry/TelemetryEvent.cpp#549
Assignee | ||
Comment 1•7 years ago
|
||
We should either:
- allow re-registering events for a category or
- ignore existing events in a category, register new ones
One concern:
If an addon registers/overwrites existing events by accidents, should we make this visible?
Should we maybe make overwriting/reregistering a warning?
Assignee | ||
Comment 2•7 years ago
|
||
Jim, you ran into this for Lockbox.
Is this a blocker for you?
If so, what is the timeline you are on?
Flags: needinfo?(squibblyflabbetydoo)
Assignee | ||
Comment 3•7 years ago
|
||
Alessio, do we have the same issue with scalars?
Do we need a bug for that?
Flags: needinfo?(alessio.placitelli)
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> We should either:
> - allow re-registering events for a category or
> - ignore existing events in a category, register new ones
Alternative: allow explicitly unregistering events.
Down-sides:
- allowing to unregister a category could unexpectedly remove other users events
- requiring to unregister individual events probably is painful
This doesn't seem like a great option.
Comment 5•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> Alessio, do we have the same issue with scalars?
> Do we need a bug for that?
Is the problem about registering different scalars/events in the same category at different times?
For example: register "telemetry.addon.first_time" (the category being "telemetry.addon") and then, after a bit or the next update, registering "telemetry.addon.second_time" (same category)?
If so, then Scalars can do that.
Flags: needinfo?(alessio.placitelli) → needinfo?(gfritzsche)
Comment 6•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> Jim, you ran into this for Lockbox.
> Is this a blocker for you?
> If so, what is the timeline you are on?
We have a workaround (we just have a versioned category named), so it's merely inconvenient to us. A better solution would be great, though. :)
Flags: needinfo?(squibblyflabbetydoo)
Comment 7•7 years ago
|
||
Seconding Jim, versioning would get ugly in the long run, so if we could have better solution before our Beta release (early Dec) it would make our lives a lot cleaner. Thanks!
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Alessio Placitelli [:Dexter] from comment #5)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> > Alessio, do we have the same issue with scalars?
> > Do we need a bug for that?
>
> Is the problem about registering different scalars/events in the same
> category at different times?
>
> For example: register "telemetry.addon.first_time" (the category being
> "telemetry.addon") and then, after a bit or the next update, registering
> "telemetry.addon.second_time" (same category)?
>
> If so, then Scalars can do that.
Great. How about:
(1) register "test.scalar1"
(2) later, register both "test.scalar1" & "test.scalar2".
Will "test.scalar2" get registered?
Flags: needinfo?(gfritzsche)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(alessio.placitelli)
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Leif Oines [:loines] from comment #7)
> Seconding Jim, versioning would get ugly in the long run, so if we could
> have better solution before our Beta release (early Dec) it would make our
> lives a lot cleaner. Thanks!
That should work out fine - we'll see that we get a fix on 58, which goes to beta on Nov 13.
Updated•7 years ago
|
Comment 10•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Great. How about:
> (1) register "test.scalar1"
> (2) later, register both "test.scalar1" & "test.scalar2".
>
> Will "test.scalar2" get registered?
Oh, I see the problem now :D Thank you for your patience. No, "test.scalar2" doesn't get registered. I filed bug 1409323 to fix that.
Updated•7 years ago
|
Flags: needinfo?(alessio.placitelli)
Assignee | ||
Updated•7 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8925360 -
Flags: review?(alessio.placitelli)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gfritzsche
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•7 years ago
|
||
Assignee | ||
Comment 13•7 years ago
|
||
I chose the following behavior here when re-registering an existing event:
- don't print any error or warning (this is usually expected behavior when updating add-ons)
- don't change the existing event information (that makes the collected data stable, if you want to change parameters better rename/version the event)
- however, when the "expiry" field is true, make sure existing events will not be recorded anymore (that's how we roll out expiring add-on events)
Jim, does that make sense from your perspective?
Flags: needinfo?(squibblyflabbetydoo)
Comment 14•7 years ago
|
||
> - don't change the existing event information (that makes the collected data stable, if you want to change parameters better rename/version the event)
This one concerns me a little bit; what if we just want to add more valid objects to an event? (Or remove some old objects.) Should we make a whole new event in that case?
> - however, when the "expiry" field is true, make sure existing events will not be recorded anymore (that's how we roll out expiring add-on events)
I haven't looked into how expiration works, so this sounds ok at a glance, but I'm not sure how we should be using it in practice.
Flags: needinfo?(squibblyflabbetydoo)
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Jim Porter (:squib) from comment #14)
> > - don't change the existing event information (that makes the collected data stable, if you want to change parameters better rename/version the event)
>
> This one concerns me a little bit; what if we just want to add more valid
> objects to an event? (Or remove some old objects.) Should we make a whole
> new event in that case?
Good point, let me rephrase a bit:
- You can add new (method, object) pairs.
- For existing (method, object) pairs you can only change expiry.
- Removing objects/methods is fine, they just stay registered until the end of the Firefox session.
How is that?
I've updated the test code here to make sure we cover this.
> > - however, when the "expiry" field is true, make sure existing events will not be recorded anymore (that's how we roll out expiring add-on events)
>
> I haven't looked into how expiration works, so this sounds ok at a glance,
> but I'm not sure how we should be using it in practice.
This is probably less relevant for one-off experiments and more relevant if you ship features as e.g. system add-ons:
When an event should not be recorded anymore, it can be set to `expired:true`. Now trying to record into it will not be an error, but instead silently ignored.
That way you can "expire" events without having to remove the code that records it right away.
Flags: needinfo?(squibblyflabbetydoo)
Assignee | ||
Comment 16•7 years ago
|
||
Attachment #8925502 -
Flags: review?(alessio.placitelli)
Assignee | ||
Updated•7 years ago
|
Attachment #8925360 -
Attachment is obsolete: true
Attachment #8925360 -
Flags: review?(alessio.placitelli)
Comment 17•7 years ago
|
||
Comment on attachment 8925502 [details] [diff] [review]
Allow to register new addon events for an existing category
Review of attachment 8925502 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good, thanks Georg! Just the optional part below, if you want to to clean it up (or file a follow-up).
::: toolkit/components/telemetry/TelemetryEvent.cpp
@@ -529,5 @@
>
> - // Check that none of the events are already registered.
> - for (auto& info : eventInfos) {
> - if (gEventNameIDMap.Get(UniqueEventName(info))) {
> - return RegisterEventResult::AlreadyRegistered;
Since we're no longer using this, can we remove the whole RegisterEventResult? See http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/toolkit/components/telemetry/TelemetryEvent.cpp#173
Attachment #8925502 -
Flags: review?(alessio.placitelli) → review+
Comment 18•7 years ago
|
||
All this sounds great! I don't foresee any issues with what you've got in the patch. Thanks for looking at this!
Flags: needinfo?(jporter+bmo)
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to Alessio Placitelli [:Dexter] from comment #17)
> Since we're no longer using this, can we remove the whole
> RegisterEventResult?
Good catch, removed this.
Comment 20•7 years ago
|
||
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82a3eab8a078
Allow to register new addon events for an existing category. r=dexter
Comment 21•7 years ago
|
||
Backed out for eslint failures in test_TelemetryEvents.js (dot notation):
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d3c53ff03d4fe22f9c78bb9377b16392061ccd
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=142799236&repo=mozilla-inbound
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js:555:10 | ["test2"] is better written in dot notation. (dot-notation)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js:573:10 | ["test1"] is better written in dot notation. (dot-notation)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js:574:10 | ["test2"] is better written in dot notation. (dot-notation)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js:591:10 | ["test2"] is better written in dot notation. (dot-notation)
Flags: needinfo?(gfritzsche)
Comment 22•7 years ago
|
||
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/618bbe87c822
Allow to register new addon events for an existing category. r=dexter
Comment 23•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gfritzsche)
You need to log in
before you can comment on or make changes to this bug.
Description
•