1313327 - Add C++ API for recording events

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

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.

Comment 1

[Thi Huynh]

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

Comment 2

[Georg Fritzsche [: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.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Removing priority to have this visible in triage.

Comment 4

[Carl Corcoran [:ccorcoran]]

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.

Comment 5

[Georg Fritzsche [:gfritzsche]]

(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

[Carl Corcoran [: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.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Yes, let's use telemetry events. The sooner, the better.

Comment 8

[Carl Corcoran [:ccorcoran]]

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?

Comment 9

[Chris H-C :chutten]

: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

[Carl Corcoran [:ccorcoran]]

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.

Comment 11

[Chris H-C :chutten]

Dropping needinfos so it'll show in Triage.

Comment 12

[Chris H-C :chutten]

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

Comment 13

[Chris H-C :chutten]

Attachment: Bug 1313327 - Add a C++ API for recording Telemetry Events. r?janerik

Comment 14

[Chris H-C :chutten]

Attachment: Bug 1313327 - Test C++ Events API r?janerik

Depends on D16877

Comment 15

[Chris H-C :chutten]

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

Replacing f? with ni?

Comment 16

[Chris H-C :chutten]

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.

Comment 17

[Chris H-C :chutten]

Attachment: Bug 1313327 - Flatten the Telemetry Event enums. r?janerik

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.

Comment 18

[Chris H-C :chutten]

Attachment: Bug 1313327 - Flatten the Telemetry Event enums. r?janerik

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.

Comment 19

[Chris H-C :chutten]

Attachment: Bug 1313327 - Add a C++ API for recording Telemetry Events. r?janerik

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

Comment 20

[Chris H-C :chutten]

Attachment: Bug 1313327 - Test C++ Events API r?janerik

Depends on D18692

Comment 21

[Chris H-C :chutten]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48342d2c4c530a2c367b85b28a004ec6a74c0ad7

Comment 22

[Chris H-C :chutten]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/bcbe1dc28725
https://hg.mozilla.org/mozilla-central/rev/2ad12527e422
https://hg.mozilla.org/mozilla-central/rev/9bf817ce243c

Comment 25

[Chris H-C :chutten]

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.