First implementation of Cobalt Telemetry core logic #551

joeltine · 2023-06-06T18:56:07Z

See go/cobalt-telemetry for design.

This PR implements Cobalt versions of all the core metrics control classes supported by //components/metrics. A future CL will add their instantiation and bootstrapping into application.cc.

A H5vcc interface is added to facilitate a callback mechanism in JavaScript to intercept metric's payloads.

For now, only UMA is supported. UKM is explicitly disabled everywhere as is anything related to Chromium experiments (i.e., Field Trials).

Note: I want to get early feedback on the core logic before adding unit tests. Once this CL is looking stable, I'll add tests.

b/280094891

Change-Id: I1f6da00b9bb269f5597c4b6b329262fc45dd0908

joeltine · 2023-06-06T18:57:17Z

Apologize for the size of this PR. I honestly couldn't think of a way to easily split it up without a bunch of intermediate changes. Everything depends on each other to work.

joeltine · 2023-06-07T21:20:37Z

Woohoo all CI checks are passing \o/. Should be g2g for review.

kaidokert · 2023-06-08T07:54:37Z

Woohoo all CI checks are passing \o/. Should be g2g for review.

I'll get my pass through done tomorrow

cobalt/h5vcc/script_callback_wrapper.h

kaidokert

Overall the structure looks fine. Could use quite a few more NOTIMPLEMENTED and DCHECK quards to avoid surprises.

Also, a number of the concerns poured into the code here should be brought into a design doc and reviewed.

cobalt/browser/metrics/cobalt_metrics_log_uploader.h

cobalt/browser/metrics/cobalt_enabled_state_provider.h

cobalt/browser/metrics/cobalt_metrics_service_client.cc

cobalt/h5vcc/h5vcc_metrics.idl

cobalt/h5vcc/script_callback_wrapper.h

components/crash/core/common/crash_key.h

components/metrics_services_manager/metrics_services_manager.cc

joeltine · 2023-06-12T18:49:12Z

First round of comments addressed and I didn't see any major structural change recommendations. As such, I'll follow-up with unit tests and a doc on the open design questions.

joeltine · 2023-06-12T20:30:59Z

Here's the design doc w/ the open questions: go/cobalt-telemetry-impl

joeltine · 2023-06-13T21:36:36Z

So I'm starting on unit and black box tests for this, but I'm realizing it's going to be a long road. Any qualms with me submitting the code as-is and following up with a PR dedicated to testing everything?

kaidokert · 2023-06-14T01:11:40Z

So I'm starting on unit and black box tests for this, but I'm realizing it's going to be a long road. Any qualms with me submitting the code as-is and following up with a PR dedicated to testing everything?

We had a testing team that should chime in on this. @hlwarriner @sideb0ard for comments.

I'd rather personally would like to see design and tests go in first. Especially because in the component layers we already disabled component tests, even though they are already there and written, e.g. b/283258321 and b/283275474 from here: https://github.com/youtube/cobalt/pull/387/files.

joeltine · 2023-06-14T01:27:17Z

So I'm starting on unit and black box tests for this, but I'm realizing it's going to be a long road. Any qualms with me submitting the code as-is and following up with a PR dedicated to testing everything?

We had a testing team that should chime in on this. @hlwarriner @sideb0ard for comments.

I'd rather personally would like to see design and tests go in first. Especially because in the component layers we already disabled component tests, even though they are already there and written, e.g. b/283258321 and b/283275474 from here: #387 (files).

Understandable. Can I do just trunk-only for now? Just psychologically it would be good to check this in and not continue to bloat this CL. I also have other CLs floating around that are based on this branch. Would be nice to have those tied to main instead and stop needing to sync those with this every time I make a change.

I will follow-up with tests on this. It's my next priority.

hlwarriner · 2023-06-14T15:31:56Z

Is this telemetry MVP something we're trying to get into Cobalt 24?

We said we wouldn't push the new unit test policy too hard until after the main Cobalt 24 deadlines pass, so I'm ok with this. My answer would probably change if this came up in a month or two, though.

sideb0ard · 2023-06-14T15:32:14Z

I will follow-up with tests on this. It's my next priority.

Cools, yeah, I'd just say make sure there is a ticket to track adding the tests

joeltine · 2023-06-14T15:35:00Z

Ya, would really like to get this into C24.

Here's a tracking bug for tests https://b.corp.google.com/issues/287248166

kaidokert

LGTM if the tests come soon

go/cobalt-telemetry. This PR implements Cobalt versions of all the core metrics control classes supported by //components/metrics. A H5vcc interface is added to facilitate a callback mechanism in Javascript to intercept metric's payloads. For now, only UMA is supported. UKM is explicitly disabled everywhere as is anything related to Chromium experiments (i.e., Field Trials). b/280094891 Change-Id: I1f6da00b9bb269f5597c4b6b329262fc45dd0908

b/280094891 Change-Id: I1e833a6947065fb7f2e6fe398c2249ca72d28007

b/280094891 Change-Id: I951ae45b7e359e0426c1e69ccdcc919e7e063842

b/280094891 Change-Id: Ib4336b1948db7ffaa380319700b2c97f317053d6

Misc changes to support testing. E.g., to avoid needing to create a V8 Script h5vcc, introduced cobalt_metrics_uploader_callback abstract class that can be mocked in tests. b/280094891 Change-Id: I0b1a19547b1be2747571fb451286ae67dd31b554

joeltine · 2023-06-15T15:21:59Z

FYI: I was able to get the unit tests done and just included them in the latest commit of this PR. PTAL. See test results: https://paste.googleplex.com/5234633187786752.

The tests required a few unique changes of their own, with the most significant being needing to wrap the H5vcc Script callback in a new interface CobaltMetricsUploaderCallback. This allowed mocking in tests as constructing a V8 object is non-trivial.

joeltine · 2023-06-15T15:22:40Z

Also, black box tests will be a follow-up PR.

kaidokert

Had a look at tests and looks great. I guess we'll now even see coverage delta on this once submitted.

Integration tests in follow-up CLs is perfectly fine.

hlwarriner · 2023-06-15T16:35:00Z

FYI: I was able to get the unit tests done and just included them in the latest commit of this PR. PTAL. See test results: https://paste.googleplex.com/5234633187786752.

The tests required a few unique changes of their own, with the most significant being needing to wrap the H5vcc Script callback in a new interface CobaltMetricsUploaderCallback. This allowed mocking in tests as constructing a V8 object is non-trivial.

Thanks for taking the time to add unit tests!

* First implementation of Cobalt Telemetry core logic, see go/cobalt-telemetry. This PR implements Cobalt versions of all the core metrics control classes supported by //components/metrics. A H5vcc interface is added to facilitate a callback mechanism in Javascript to intercept metric's payloads. For now, only UMA is supported. UKM is explicitly disabled everywhere as is anything related to Chromium experiments (i.e., Field Trials). b/280094891 Change-Id: I1f6da00b9bb269f5597c4b6b329262fc45dd0908 Former-commit-id: 73f2621

* First implementation of Cobalt Telemetry core logic, see go/cobalt-telemetry. This PR implements Cobalt versions of all the core metrics control classes supported by //components/metrics. A H5vcc interface is added to facilitate a callback mechanism in Javascript to intercept metric's payloads. For now, only UMA is supported. UKM is explicitly disabled everywhere as is anything related to Chromium experiments (i.e., Field Trials). b/280094891 Change-Id: I1f6da00b9bb269f5597c4b6b329262fc45dd0908 (cherry picked from commit 73f2621)

…gic (#729) * First implementation of Cobalt Telemetry core logic, see go/cobalt-telemetry. This PR implements Cobalt versions of all the core metrics control classes supported by //components/metrics. A H5vcc interface is added to facilitate a callback mechanism in Javascript to intercept metric's payloads. For now, only UMA is supported. UKM is explicitly disabled everywhere as is anything related to Chromium experiments (i.e., Field Trials). b/280094891 Change-Id: I1f6da00b9bb269f5597c4b6b329262fc45dd0908 (cherry picked from commit 73f2621) Co-authored-by: Joel Martinez <[email protected]>

These protos were added in th PR youtube#551 for the first introduction of Cobalt Telemetry. They are currently unused in Cobalt and removing them seems to reduce the binary size significantly (per bloaty analysis). b/290819695 Change-Id: I350742ea55621b29fa3d565618098791f666c141

These protos were added in th PR #551 for the first introduction of Cobalt Telemetry. They are currently unused in Cobalt and removing them seems to reduce the binary size significantly (per bloaty analysis). Protos removed: omnibox_event.proto, sampled_profile.proto, memory_leak.proto. b/290819695 Change-Id: I350742ea55621b29fa3d565618098791f666c141

These protos were added in th PR #551 for the first introduction of Cobalt Telemetry. They are currently unused in Cobalt and removing them seems to reduce the binary size significantly (per bloaty analysis). Protos removed: omnibox_event.proto, sampled_profile.proto, memory_leak.proto. b/290819695 Change-Id: I350742ea55621b29fa3d565618098791f666c141 (cherry picked from commit eb78ed1)

…ary size increase (#1029) Refer to the original PR: #1025 These protos were added in th PR #551 for the first introduction of Cobalt Telemetry. They are currently unused in Cobalt and removing them seems to reduce the binary size significantly (per bloaty analysis). Protos removed: omnibox_event.proto, sampled_profile.proto, memory_leak.proto. b/290819695 Change-Id: I350742ea55621b29fa3d565618098791f666c141 Co-authored-by: Joel Martinez <[email protected]>

joeltine requested a review from kaidokert June 6, 2023 18:56

joeltine force-pushed the cobalt-telemetry branch 9 times, most recently from 6c86368 to 6402f94 Compare June 7, 2023 20:51

kaidokert requested review from jellefoks and y4vor June 8, 2023 07:55

y4vor approved these changes Jun 9, 2023

View reviewed changes

cobalt/h5vcc/script_callback_wrapper.h Outdated Show resolved Hide resolved

kaidokert requested changes Jun 9, 2023

View reviewed changes

joeltine force-pushed the cobalt-telemetry branch from 6402f94 to 64d1200 Compare June 12, 2023 17:29

joeltine force-pushed the cobalt-telemetry branch from 4995e03 to 8382465 Compare June 12, 2023 18:54

kaidokert approved these changes Jun 14, 2023

View reviewed changes

joeltine added 2 commits June 15, 2023 07:42

Resolve first round of PR comments.

714bbe2

b/280094891 Change-Id: I1e833a6947065fb7f2e6fe398c2249ca72d28007

joeltine added 2 commits June 15, 2023 07:42

Make sure metrics are in a disabled state by default.

834f7aa

b/280094891 Change-Id: I951ae45b7e359e0426c1e69ccdcc919e7e063842

Improve metric toggle idl docs

fff1f10

b/280094891 Change-Id: Ib4336b1948db7ffaa380319700b2c97f317053d6

joeltine force-pushed the cobalt-telemetry branch from 9ab01f1 to fff1f10 Compare June 15, 2023 13:43

Add unit tests

2ebb8d5

Misc changes to support testing. E.g., to avoid needing to create a V8 Script h5vcc, introduced cobalt_metrics_uploader_callback abstract class that can be mocked in tests. b/280094891 Change-Id: I0b1a19547b1be2747571fb451286ae67dd31b554

kaidokert approved these changes Jun 15, 2023

View reviewed changes

andrewsavage1 approved these changes Jun 15, 2023

View reviewed changes

joeltine merged commit 73f2621 into youtube:main Jun 15, 2023

joeltine deleted the cobalt-telemetry branch June 15, 2023 17:34

joeltine added the cp-24.lts.1+ Cherry Pick to the 24.lts.1+ branch label Jun 27, 2023

cobalt-github-releaser-bot mentioned this pull request Jun 27, 2023

Cherry pick PR #551: First implementation of Cobalt Telemetry core logic #729

Merged

joeltine mentioned this pull request Jul 24, 2023

Remove unused UMA protos to minimize Cobalt binary size increase #1025

Merged

cobalt-github-releaser-bot mentioned this pull request Jul 24, 2023

Cherry pick PR #1025: Remove unused UMA protos to minimize Cobalt binary size increase #1029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First implementation of Cobalt Telemetry core logic #551

First implementation of Cobalt Telemetry core logic #551

joeltine commented Jun 6, 2023 •

edited

Loading

joeltine commented Jun 6, 2023

joeltine commented Jun 7, 2023

kaidokert commented Jun 8, 2023

kaidokert left a comment

joeltine commented Jun 12, 2023

joeltine commented Jun 12, 2023

joeltine commented Jun 13, 2023

kaidokert commented Jun 14, 2023

joeltine commented Jun 14, 2023

hlwarriner commented Jun 14, 2023

sideb0ard commented Jun 14, 2023

joeltine commented Jun 14, 2023

kaidokert left a comment

joeltine commented Jun 15, 2023

joeltine commented Jun 15, 2023

kaidokert left a comment

hlwarriner commented Jun 15, 2023

First implementation of Cobalt Telemetry core logic #551

First implementation of Cobalt Telemetry core logic #551

Conversation

joeltine commented Jun 6, 2023 • edited Loading

joeltine commented Jun 6, 2023

joeltine commented Jun 7, 2023

kaidokert commented Jun 8, 2023

kaidokert left a comment

Choose a reason for hiding this comment

joeltine commented Jun 12, 2023

joeltine commented Jun 12, 2023

joeltine commented Jun 13, 2023

kaidokert commented Jun 14, 2023

joeltine commented Jun 14, 2023

hlwarriner commented Jun 14, 2023

sideb0ard commented Jun 14, 2023

joeltine commented Jun 14, 2023

kaidokert left a comment

Choose a reason for hiding this comment

joeltine commented Jun 15, 2023

joeltine commented Jun 15, 2023

kaidokert left a comment

Choose a reason for hiding this comment

hlwarriner commented Jun 15, 2023

joeltine commented Jun 6, 2023 •

edited

Loading