Add C++ API for recording events
Categories
(Toolkit :: Telemetry, defect, P1)
Tracking
()
People
(Reporter: gfritzsche, Assigned: chutten)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [measurement:client])
Attachments
(3 files, 3 obsolete files)
While bug 1302663 adds a JS API for recording events, we will need to support recording them from C++ as well. With patches on bug 1305648 coming up, we should start out with test coverage for that API right away.
(In reply to Georg Fritzsche [:gfritzsche] from comment #0) > While bug 1302663 adds a JS API for recording events, we will need to > support recording them from C++ as well. > > With patches on bug 1305648 coming up, we should start out with test > coverage for that API right away. Hi, I want to work on this but I have some concerns. From what I see in the bug 1302663, we already have the code for recording events, then what are the use cases for the this C++ API. I mean, could you explain or give me an example of how this C++ API will be used?
Reporter | ||
Comment 2•6 years ago
|
||
Hi, thanks for checking in on this. Since we added events, we didn't have any requests asking for this C++ API - so this is not something we need to do currently.
Comment 4•6 years ago
|
||
A current use case is for bug 1372827: Mozglue blocks some threads from executing if it looks suspicious. We want to gather telemetry when that happens, so it would be very natural to call something like Telemetry::RecordEvent() from C++ land.
Reporter | ||
Comment 5•6 years ago
|
||
(In reply to Carl Corcoran [:ccorcoran] from comment #4) > A current use case is for bug 1372827: Mozglue blocks some threads from > executing if it looks suspicious. We want to gather telemetry when that > happens, so it would be very natural to call something like > Telemetry::RecordEvent() from C++ land. We can definitely add this API, we just didn't have a motivating use-case before. What is the timeline you need this for?
Comment 6•6 years ago
|
||
I'm not sure of a timeline, and yesterday I opened discussion with aklotz about possibly using a custom ping instead of telemetry events for this. I'll let aklotz answer this.
Comment 7•5 years ago
|
||
Yes, let's use telemetry events. The sooner, the better.
Comment 8•5 years ago
|
||
Georg we're eager to move forward using this. It would be helpful to know around when it will be available. Do you have a rough time estimate?
Assignee | ||
Comment 9•5 years ago
|
||
:aklotz, :ccorcoran, could you document the initial usecases you wish to use this for? That'll help us understand the priority and how best to help you.
Comment 10•5 years ago
|
||
Our specific use case is bug 1372827: We would like to use telemetry events to record when we block suspicious-looking threads from starting. Currently we do the blocking but have no telemetry insight.
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
Dropping needinfos so it'll show in Triage.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
I more or less have this written, though the test would have me believe otherwise.
Assignee | ||
Comment 13•5 years ago
|
||
Assignee | ||
Comment 14•5 years ago
|
||
Depends on D16877
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 15•5 years ago
|
||
Comment on attachment 9037306 [details]
Bug 1313327 - Add a C++ API for recording Telemetry Events. r?janerik
Replacing f? with ni?
Assignee | ||
Comment 16•5 years ago
|
||
Extreme cold this week kept me from finishing the patch (it's on a computer under a desk at my coworking location which I've been unable to cycle to). The weather will have less of a "frostbite in minutes" character by Tuesday when I expect to be able to finish it, update the review docs to prefer enums over string-based APIs, and pass it to try.
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 17•5 years ago
|
||
To support a non-templated API for recording events by enum, all the enum
values must be of the same type. Inheritence doesn't really seem to be a thing
in C++ enums, so that means flattening them all into a single, big enum the
way Scalars and Histograms work.
Alas.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 18•5 years ago
|
||
To support a non-templated API for recording events by enum, all the enum
values must be of the same type. Inheritence doesn't really seem to be a thing
in C++ enums, so that means flattening them all into a single, big enum the
way Scalars and Histograms work.
Alas.
Assignee | ||
Comment 19•5 years ago
|
||
This isn't the most efficient implementation as it uses the enum parameter to
fetch the strings and then uses internal string APIs (which then get the enum
again), but it's the most straightforward impl with a smallish patch.
If this starts showing on profiles we can look into efficiency then.
Depends on D18691
Assignee | ||
Comment 20•5 years ago
|
||
Depends on D18692
Assignee | ||
Comment 21•5 years ago
|
||
Assignee | ||
Comment 22•5 years ago
|
||
Do you require a C++ API to enable the category to be able to be recorded, or is that something you'll be able to do from JS?
Comment 23•5 years ago
|
||
Pushed by chutten@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bcbe1dc28725 Flatten the Telemetry Event enums. r=janerik https://hg.mozilla.org/integration/autoland/rev/2ad12527e422 Add a C++ API for recording Telemetry Events. r=janerik https://hg.mozilla.org/integration/autoland/rev/9bf817ce243c Test C++ Events API r=janerik
Comment 24•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bcbe1dc28725
https://hg.mozilla.org/mozilla-central/rev/2ad12527e422
https://hg.mozilla.org/mozilla-central/rev/9bf817ce243c
Assignee | ||
Comment 25•5 years ago
|
||
I gave you a C++ API for enabling because I was in the neighbourhood :)
Please let me know if you have any questions about using this.
Updated•5 years ago
|
Description
•