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

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?

Jim, you ran into this for Lockbox. Is this a blocker for you? If so, what is the timeline you are on?

Alessio, do we have the same issue with scalars? Do we need a bug for that?

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

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

(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. :)

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!

(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?

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

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

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?

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

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

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

All this sounds great! I don't foresee any issues with what you've got in the patch. Thanks for looking at this!

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

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

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)

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