Add C++ API for recording events

RESOLVED FIXED in Firefox 67

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: gfritzsche, Assigned: chutten)

Tracking

(Blocks 2 bugs)

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox67 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(3 attachments, 3 obsolete attachments)

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?
Flags: needinfo?(gfritzsche)
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.
Flags: needinfo?(gfritzsche)
Priority: P3 → P4
Removing priority to have this visible in triage.
Priority: P4 → --
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.
(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?
Flags: needinfo?(ccorcoran)
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.
Flags: needinfo?(ccorcoran) → needinfo?(aklotz)
Yes, let's use telemetry events. The sooner, the better.
Flags: needinfo?(aklotz)
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?
Flags: needinfo?(gfritzsche)
: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.
Flags: needinfo?(ccorcoran)
Flags: needinfo?(aklotz)
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.
Flags: needinfo?(ccorcoran)
Flags: needinfo?(aklotz)
Flags: needinfo?(chutten)
Dropping needinfos so it'll show in Triage.
Flags: needinfo?(gfritzsche)
Flags: needinfo?(chutten)
Priority: -- → P2
Assignee: nobody → chutten
Status: NEW → ASSIGNED
Priority: P2 → P1

I more or less have this written, though the test would have me believe otherwise.

Depends on D16877

Attachment #9037306 - Flags: feedback?(gfritzsche)

Comment on attachment 9037306 [details]
Bug 1313327 - Add a C++ API for recording Telemetry Events. r?janerik

Replacing f? with ni?

Flags: needinfo?(gfritzsche)
Attachment #9037306 - Flags: feedback?(gfritzsche)

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.

Flags: needinfo?(gfritzsche)

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.

Attachment #9037307 - Attachment is obsolete: true
Attachment #9037306 - Attachment is obsolete: true
Attachment #9041461 - Attachment is obsolete: true

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.

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

Depends on D18692

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?

Flags: needinfo?(ccorcoran)
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
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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.

Flags: needinfo?(ccorcoran)
You need to log in before you can comment on or make changes to this bug.