Sangbaek Park added 1 comment

Sangbaek Park . unresolved

@dalec...@chromium.org

1. `Media.WebMediaPlayerState` already reports `RendererType` field whereas `Media.BasicPlayback` doesn't.
2. `Media.WebMediaPlayerState` has `KeySystem` and `IsHardwareSecure` fields whereas `Media.BasicPlayback` doesn't.
3. As far as MediaFoundationRenderer is concerned, we have to do a join operation between two UKMs on the same PlayerID to inspect some UKM fields from `Media.BasicPlayback`.

Not sure it would be more proper if we add `KeySystem` and `IsHardwareSecure` to `Media.BasicPlayback` instead (or not sure these values are available for the watch time recorder). By simply reporting `RendererType` field for `Media.BasicPlayback`, it gives us more flexibility without the expensive join operation when inspecting the data. WDYT?

Related details

Attention is currently required from:

Dale Curtis

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Vikram Pasupathy (Gerrit)

unread,

Sep 18, 2025, 7:25:57 PMSep 18

to Sangbaek Park, Chromium LUCI CQ, Dale Curtis, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Dale Curtis

Vikram Pasupathy added 1 comment

Patchset-level comments

Sangbaek Park . unresolved

@dalec...@chromium.org
1. `Media.WebMediaPlayerState` already reports `RendererType` field whereas `Media.BasicPlayback` doesn't.
2. `Media.WebMediaPlayerState` has `KeySystem` and `IsHardwareSecure` fields whereas `Media.BasicPlayback` doesn't.
3. As far as MediaFoundationRenderer is concerned, we have to do a join operation between two UKMs on the same PlayerID to inspect some UKM fields from `Media.BasicPlayback`.
Not sure it would be more proper if we add `KeySystem` and `IsHardwareSecure` to `Media.BasicPlayback` instead (or not sure these values are available for the watch time recorder). By simply reporting `RendererType` field for `Media.BasicPlayback`, it gives us more flexibility without the expensive join operation when inspecting the data. WDYT?

Vikram Pasupathy

+1 to this idea, and two follow up questions for Dale:

1. Are there any docs discussing the differences and intentions with the different Media Playback UKMS? I don't understand what feels like some duplication (which we try solving via ideas like Sangbaek's). Maybe there is something out there, but couldn't find anything after a cursory search.

2. When we add a field like this to the UKM, are we expected to create a new UKM review document? ex: https://docs.google.com/document/d/1cdfAP66Hm4W4hcXmgOm1BqScYIS0KP7cyCgLu91f-kE/edit?tab=t.0#heading=h.k5jx6iluw4yt

I'm asking because I've seen UKMs have a running document where reviewers can give LGTMs to fields, rather than explaining what the UKM does from scratch.

Related details

Attention is currently required from:

Dale Curtis

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dale Curtis (Gerrit)

unread,

Sep 19, 2025, 3:01:26 PMSep 19

to Sangbaek Park, Chromium LUCI CQ, Vikram Pasupathy, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Sangbaek Park

Dale Curtis added 1 comment

Patchset-level comments

Sangbaek Park . unresolved

@dalec...@chromium.org
1. `Media.WebMediaPlayerState` already reports `RendererType` field whereas `Media.BasicPlayback` doesn't.
2. `Media.WebMediaPlayerState` has `KeySystem` and `IsHardwareSecure` fields whereas `Media.BasicPlayback` doesn't.
3. As far as MediaFoundationRenderer is concerned, we have to do a join operation between two UKMs on the same PlayerID to inspect some UKM fields from `Media.BasicPlayback`.
Not sure it would be more proper if we add `KeySystem` and `IsHardwareSecure` to `Media.BasicPlayback` instead (or not sure these values are available for the watch time recorder). By simply reporting `RendererType` field for `Media.BasicPlayback`, it gives us more flexibility without the expensive join operation when inspecting the data. WDYT?

Vikram Pasupathy

+1 to this idea, and two follow up questions for Dale:
1. Are there any docs discussing the differences and intentions with the different Media Playback UKMS? I don't understand what feels like some duplication (which we try solving via ideas like Sangbaek's). Maybe there is something out there, but couldn't find anything after a cursory search.
2. When we add a field like this to the UKM, are we expected to create a new UKM review document? ex: https://docs.google.com/document/d/1cdfAP66Hm4W4hcXmgOm1BqScYIS0KP7cyCgLu91f-kE/edit?tab=t.0#heading=h.k5jx6iluw4yt
I'm asking because I've seen UKMs have a running document where reviewers can give LGTMs to fields, rather than explaining what the UKM does from scratch.

Dale Curtis

Anything that can change is supposed to be on the BaiscPlayback table, WebMediaPlayerState is supposed to be one time. RendererType is a bit of an exception though since it can actually change (e.g., remoting renderer). So it should actually have been on the basic playback table all along.

I don't understand what's so problematic about the JOIN though? Presumably you're just using an inner join between the two tables (after filtering down to just what you want in basic playback table) based on the id?

Related details

Attention is currently required from:

Sangbaek Park

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sangbaek Park (Gerrit)

unread,

Sep 19, 2025, 3:51:12 PMSep 19

to Chromium LUCI CQ, Dale Curtis, Vikram Pasupathy, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Dale Curtis and Vikram Pasupathy

Sangbaek Park added 1 comment

Patchset-level comments

Sangbaek Park . unresolved

@dalec...@chromium.org
1. `Media.WebMediaPlayerState` already reports `RendererType` field whereas `Media.BasicPlayback` doesn't.
2. `Media.WebMediaPlayerState` has `KeySystem` and `IsHardwareSecure` fields whereas `Media.BasicPlayback` doesn't.
3. As far as MediaFoundationRenderer is concerned, we have to do a join operation between two UKMs on the same PlayerID to inspect some UKM fields from `Media.BasicPlayback`.
Not sure it would be more proper if we add `KeySystem` and `IsHardwareSecure` to `Media.BasicPlayback` instead (or not sure these values are available for the watch time recorder). By simply reporting `RendererType` field for `Media.BasicPlayback`, it gives us more flexibility without the expensive join operation when inspecting the data. WDYT?

Vikram Pasupathy

+1 to this idea, and two follow up questions for Dale:
1. Are there any docs discussing the differences and intentions with the different Media Playback UKMS? I don't understand what feels like some duplication (which we try solving via ideas like Sangbaek's). Maybe there is something out there, but couldn't find anything after a cursory search.
2. When we add a field like this to the UKM, are we expected to create a new UKM review document? ex: https://docs.google.com/document/d/1cdfAP66Hm4W4hcXmgOm1BqScYIS0KP7cyCgLu91f-kE/edit?tab=t.0#heading=h.k5jx6iluw4yt
I'm asking because I've seen UKMs have a running document where reviewers can give LGTMs to fields, rather than explaining what the UKM does from scratch.

Dale Curtis

Anything that can change is supposed to be on the BaiscPlayback table, WebMediaPlayerState is supposed to be one time. RendererType is a bit of an exception though since it can actually change (e.g., remoting renderer). So it should actually have been on the basic playback table all along.
I don't understand what's so problematic about the JOIN though? Presumably you're just using an inner join between the two tables (after filtering down to just what you want in basic playback table) based on the id?

Sangbaek Park

Presumably you're just using an inner join between the two tables (after filtering down to just what you want in basic playback table) based on the id?

Right. Actually, it's not a huge deal for doing that inner join operation but it gives us a good benefit when tackling some playback issues specifically for PlayReady HWDRM + detecting abuse/fraud cases if RendererType is reported in the BasicPlayback. And also I'm not sure if selecting the same PlayerID between BasicPlayback and WebMediaPlayerState per session_id per client_id gives us the correct entries since the PlayerID is an incremental number from 0 (any idea on this?).

Looks like adding `RendererType` into BasicPlayback is good to go, right?

Related details

Attention is currently required from:

Dale Curtis
Vikram Pasupathy

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dale Curtis (Gerrit)

unread,

Sep 19, 2025, 4:05:26 PMSep 19

to Sangbaek Park, Chromium LUCI CQ, Vikram Pasupathy, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Sangbaek Park and Vikram Pasupathy

Dale Curtis added 1 comment

Patchset-level comments

Sangbaek Park . unresolved

@dalec...@chromium.org
1. `Media.WebMediaPlayerState` already reports `RendererType` field whereas `Media.BasicPlayback` doesn't.
2. `Media.WebMediaPlayerState` has `KeySystem` and `IsHardwareSecure` fields whereas `Media.BasicPlayback` doesn't.
3. As far as MediaFoundationRenderer is concerned, we have to do a join operation between two UKMs on the same PlayerID to inspect some UKM fields from `Media.BasicPlayback`.
Not sure it would be more proper if we add `KeySystem` and `IsHardwareSecure` to `Media.BasicPlayback` instead (or not sure these values are available for the watch time recorder). By simply reporting `RendererType` field for `Media.BasicPlayback`, it gives us more flexibility without the expensive join operation when inspecting the data. WDYT?

Vikram Pasupathy

+1 to this idea, and two follow up questions for Dale:
1. Are there any docs discussing the differences and intentions with the different Media Playback UKMS? I don't understand what feels like some duplication (which we try solving via ideas like Sangbaek's). Maybe there is something out there, but couldn't find anything after a cursory search.
2. When we add a field like this to the UKM, are we expected to create a new UKM review document? ex: https://docs.google.com/document/d/1cdfAP66Hm4W4hcXmgOm1BqScYIS0KP7cyCgLu91f-kE/edit?tab=t.0#heading=h.k5jx6iluw4yt
I'm asking because I've seen UKMs have a running document where reviewers can give LGTMs to fields, rather than explaining what the UKM does from scratch.

Dale Curtis

Anything that can change is supposed to be on the BaiscPlayback table, WebMediaPlayerState is supposed to be one time. RendererType is a bit of an exception though since it can actually change (e.g., remoting renderer). So it should actually have been on the basic playback table all along.
I don't understand what's so problematic about the JOIN though? Presumably you're just using an inner join between the two tables (after filtering down to just what you want in basic playback table) based on the id?

Sangbaek Park

Presumably you're just using an inner join between the two tables (after filtering down to just what you want in basic playback table) based on the id?
Right. Actually, it's not a huge deal for doing that inner join operation but it gives us a good benefit when tackling some playback issues specifically for PlayReady HWDRM + detecting abuse/fraud cases if RendererType is reported in the BasicPlayback. And also I'm not sure if selecting the same PlayerID between BasicPlayback and WebMediaPlayerState per session_id per client_id gives us the correct entries since the PlayerID is an incremental number from 0 (any idea on this?).
Looks like adding `RendererType` into BasicPlayback is good to go, right?

Dale Curtis

Right you have to join on client_id+session_id+player_id. I'm not opposed to it, but you'll need UKM review to decide if we need a new doc for copying this key to another table. Probably we should deprecate the one in WMPState since renderer can change, but that would mean updating the code a bit.

Related details

Attention is currently required from:

Sangbaek Park
Vikram Pasupathy

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sangbaek Park (Gerrit)

unread,

Sep 22, 2025, 12:54:01 PMSep 22

to Chromium LUCI CQ, Dale Curtis, Vikram Pasupathy, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Dale Curtis and Vikram Pasupathy

Sangbaek Park added 1 comment

Patchset-level comments

Sangbaek Park . unresolved

@dalec...@chromium.org
1. `Media.WebMediaPlayerState` already reports `RendererType` field whereas `Media.BasicPlayback` doesn't.
2. `Media.WebMediaPlayerState` has `KeySystem` and `IsHardwareSecure` fields whereas `Media.BasicPlayback` doesn't.
3. As far as MediaFoundationRenderer is concerned, we have to do a join operation between two UKMs on the same PlayerID to inspect some UKM fields from `Media.BasicPlayback`.
Not sure it would be more proper if we add `KeySystem` and `IsHardwareSecure` to `Media.BasicPlayback` instead (or not sure these values are available for the watch time recorder). By simply reporting `RendererType` field for `Media.BasicPlayback`, it gives us more flexibility without the expensive join operation when inspecting the data. WDYT?

Vikram Pasupathy

+1 to this idea, and two follow up questions for Dale:
1. Are there any docs discussing the differences and intentions with the different Media Playback UKMS? I don't understand what feels like some duplication (which we try solving via ideas like Sangbaek's). Maybe there is something out there, but couldn't find anything after a cursory search.
2. When we add a field like this to the UKM, are we expected to create a new UKM review document? ex: https://docs.google.com/document/d/1cdfAP66Hm4W4hcXmgOm1BqScYIS0KP7cyCgLu91f-kE/edit?tab=t.0#heading=h.k5jx6iluw4yt
I'm asking because I've seen UKMs have a running document where reviewers can give LGTMs to fields, rather than explaining what the UKM does from scratch.

Dale Curtis

Anything that can change is supposed to be on the BaiscPlayback table, WebMediaPlayerState is supposed to be one time. RendererType is a bit of an exception though since it can actually change (e.g., remoting renderer). So it should actually have been on the basic playback table all along.
I don't understand what's so problematic about the JOIN though? Presumably you're just using an inner join between the two tables (after filtering down to just what you want in basic playback table) based on the id?

Sangbaek Park

Presumably you're just using an inner join between the two tables (after filtering down to just what you want in basic playback table) based on the id?
Right. Actually, it's not a huge deal for doing that inner join operation but it gives us a good benefit when tackling some playback issues specifically for PlayReady HWDRM + detecting abuse/fraud cases if RendererType is reported in the BasicPlayback. And also I'm not sure if selecting the same PlayerID between BasicPlayback and WebMediaPlayerState per session_id per client_id gives us the correct entries since the PlayerID is an incremental number from 0 (any idea on this?).
Looks like adding `RendererType` into BasicPlayback is good to go, right?

Dale Curtis

Right you have to join on client_id+session_id+player_id. I'm not opposed to it, but you'll need UKM review to decide if we need a new doc for copying this key to another table. Probably we should deprecate the one in WMPState since renderer can change, but that would mean updating the code a bit.

Sangbaek Park

Thank you for your time again! I can go through a new UKM doc and review but sounds like I need a plan like adding rendering type to BasicPlayback and deprecating the existing on from WMPState.

And I noticed this CL doesn't report RendererType as MediaFoundationRenderer because my guess is we don't recreate a watch time component upon the renderer type change (I have another bug and added a note: b/302699189#comment3).

My current plan:

1. Fix this renderer type component recreation issue (b/302699189) first
2. Write a new UKM doc and review
3. Add RendererType to BasicPlayback
4. Deprecate RendererType from WMPState

(Note that 2-4 have a low priority for now since it's more like a nice-to-have feature).

FYI; On Windows local machine, nothing is reported in chrome://ukm mostly due to the finch flag (I guess throttling due to high traffics on Windows) so we have to override the UkmSamplingRate with `-enable-feature=UkmSamplingRate`. For details, see b/446238506. Now I am able to verify UKMs in chrome://ukm on my local Windows debug build.

Related details

Attention is currently required from:

Dale Curtis
Vikram Pasupathy

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dale Curtis (Gerrit)

unread,

Sep 22, 2025, 1:09:56 PMSep 22

to Sangbaek Park, Chromium LUCI CQ, Vikram Pasupathy, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Sangbaek Park and Vikram Pasupathy

Dale Curtis added 1 comment

Patchset-level comments

Dale Curtis

I'd check with a UKM reviewer if 2 is needed first, but otherwise sgtm. Thanks!

Related details

Attention is currently required from:

Sangbaek Park
Vikram Pasupathy

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sangbaek Park (Gerrit)

unread,

Oct 16, 2025, 1:07:59 PM (4 days ago) Oct 16

to Chromium LUCI CQ, Dale Curtis, Vikram Pasupathy, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Dale Curtis and Vikram Pasupathy

Sangbaek Park added 1 comment

Patchset-level comments

Sangbaek Park

I fixed hte blocking issue (RendererType is not updated to the watch time recorder) by http://crrev.com/c/7047462 and rebased this CL.

Tested locally. Verified via UKM logs: http://shortn/_Ava9MN59qO

Now I am about to ask UKM owners if this needs a UKM doc review or not.

@dalec...@chromium.org, do you have the previously reviewed UKM doc for Media.BasicPlayback and/or Media.WebMediaPlayerState? I'd like ask them if I can reuse it.

Related details

Attention is currently required from:

Dale Curtis
Vikram Pasupathy

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dale Curtis (Gerrit)

unread,

Oct 16, 2025, 1:14:22 PM (4 days ago) Oct 16

to Sangbaek Park, Chromium LUCI CQ, Vikram Pasupathy, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Sangbaek Park and Vikram Pasupathy

Dale Curtis added 1 comment

Patchset-level comments

Dale Curtis

Sent via chat!

Related details

Attention is currently required from:

Sangbaek Park
Vikram Pasupathy

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sangbaek Park (Gerrit)

unread,

Oct 16, 2025, 1:46:03 PM (4 days ago) Oct 16

to Chromium Metrics Reviews, Chromium LUCI CQ, Dale Curtis, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Dale Curtis and Vikram Pasupathy

Sangbaek Park added 1 comment

Patchset-level comments

@chromium-met...@google.com,

Sangbaek Park

We have the previously approved UKM doc (http://shortn/_BJ13xcmzEx) for both Media.BasicPlayback and Media.WebMediaPlayerState.

The current CL is trying to add `RendererType` field to the Media.BasicPlayback UKM. The same field already exists in the Media.WebMediaPlayerState UKM.

UKM owners, if I need a UKM doc for this CL, can I reuse the previous UKM doc for reviews?

Related details

Attention is currently required from:

Dale Curtis
Vikram Pasupathy

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

gwsq (Gerrit)

unread,

Oct 16, 2025, 1:47:21 PM (4 days ago) Oct 16

to Sangbaek Park, Chromium Metrics Reviews, Sun Yueru, Chromium LUCI CQ, Dale Curtis, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Dale Curtis, Sun Yueru and Vikram Pasupathy

Message from gwsq

Reviewer source(s):
yr...@chromium.org is from context(analysis/uma/chrome-metrics.gwsq)

Related details

Attention is currently required from:

Dale Curtis
Sun Yueru
Vikram Pasupathy

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sangbaek Park (Gerrit)

unread,

Oct 17, 2025, 12:02:46 PM (3 days ago) Oct 17

to Chromium Metrics Reviews, Sun Yueru, Chromium LUCI CQ, Dale Curtis, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Dale Curtis, Sun Yueru and Vikram Pasupathy

Patchset-level comments

Sangbaek Park

@yr...@chromium.org, Could you please check out my previous question above? Thank you!

Related details

Attention is currently required from:

Dale Curtis
Sun Yueru
Vikram Pasupathy

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sun Yueru (Gerrit)

unread,

Oct 17, 2025, 6:02:23 PM (3 days ago) Oct 17

to Sangbaek Park, Chromium Metrics Reviews, Chromium LUCI CQ, Dale Curtis, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Dale Curtis, Sangbaek Park and Vikram Pasupathy

Sun Yueru added 2 comments

Patchset-level comments

Sangbaek Park . resolved

Sun Yueru

The approved doc does say the proposal is to add renderer_type to the two event types, so that's already covered.

File tools/metrics/ukm/ukm.xml

Line 14192, Patchset 4 (Latest): <aggregation>

Sun Yueru . unresolved

This is for making this field available on go/ukm-dash, I suspect you don't need that since none of the other fields are aggregated, and I recall seeing a custom (Plx?) dash from Dale for viewing media metrics.

Otherwise LGTM.

Related details

Attention is currently required from:

Dale Curtis
Sangbaek Park
Vikram Pasupathy

satisfied_requirement

unsatisfied_requirement

open

diffy

Sangbaek Park (Gerrit)

unread,

Oct 17, 2025, 6:20:40 PM (3 days ago) Oct 17

to Chromium Metrics Reviews, Sun Yueru, Chromium LUCI CQ, Dale Curtis, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Dale Curtis, Sun Yueru and Vikram Pasupathy

Sangbaek Park added 3 comments

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Sangbaek Park

Ack. Thank you!

Sangbaek Park . resolved

@dalec...@chromium.org PTAL, thank you!

File tools/metrics/ukm/ukm.xml

Line 14192, Patchset 4 (Latest): <aggregation>

Sun Yueru . unresolved

This is for making this field available on go/ukm-dash, I suspect you don't need that since none of the other fields are aggregated, and I recall seeing a custom (Plx?) dash from Dale for viewing media metrics.
Otherwise LGTM.

Sangbaek Park

You're right. It's not aggregated so it won't be showing on go/ukm-dash. This field is added for PLX script queries when needed.

Related details

Attention is currently required from:

Dale Curtis
Sun Yueru
Vikram Pasupathy

satisfied_requirement

unsatisfied_requirement

open

diffy

Dale Curtis (Gerrit)

unread,

Oct 17, 2025, 7:25:18 PM (3 days ago) Oct 17

to Sangbaek Park, Chromium Metrics Reviews, Sun Yueru, Chromium LUCI CQ, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Sangbaek Park and Sun Yueru

Dale Curtis voted Code-Review+1

Code-Review

+1

Related details

Attention is currently required from:

Sangbaek Park
Sun Yueru

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sangbaek Park (Gerrit)

unread,

11:18 AM (9 hours ago) 11:18 AM

to Dale Curtis, Chromium Metrics Reviews, Sun Yueru, Chromium LUCI CQ, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Sun Yueru

Sangbaek Park added 1 comment

File tools/metrics/ukm/ukm.xml

Line 14192, Patchset 4 (Latest): <aggregation>

Sun Yueru . unresolved

This is for making this field available on go/ukm-dash, I suspect you don't need that since none of the other fields are aggregated, and I recall seeing a custom (Plx?) dash from Dale for viewing media metrics.
Otherwise LGTM.

Sangbaek Park

You're right. It's not aggregated so it won't be showing on go/ukm-dash. This field is added for PLX script queries when needed.

Sangbaek Park

@yr...@chromium.org, Dale stampled so PTAL again. Thank you!

Related details

Attention is currently required from:

Sun Yueru

satisfied_requirement

unsatisfied_requirement

open

diffy

Sun Yueru (Gerrit)

unread,

1:27 PM (6 hours ago) 1:27 PM

to Sangbaek Park, Dale Curtis, Chromium Metrics Reviews, Chromium LUCI CQ, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Sangbaek Park

Sun Yueru added 1 comment

File tools/metrics/ukm/ukm.xml

Line 14192, Patchset 4 (Latest): <aggregation>

Sun Yueru . unresolved

This is for making this field available on go/ukm-dash, I suspect you don't need that since none of the other fields are aggregated, and I recall seeing a custom (Plx?) dash from Dale for viewing media metrics.
Otherwise LGTM.

Sangbaek Park

You're right. It's not aggregated so it won't be showing on go/ukm-dash. This field is added for PLX script queries when needed.

Sangbaek Park

@yr...@chromium.org, Dale stampled so PTAL again. Thank you!

Sun Yueru

It's not aggregated

This field is added for PLX script queries when needed.

The plx script doesn't read from the aggregated table, does it? If no one actually needs the <aggregation>, let's remove it if it's not used. Having <aggregation> incurs additional processing costs for UKM pipelines.

Related details

Attention is currently required from:

Sangbaek Park

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sangbaek Park (Gerrit)

unread,

1:52 PM (6 hours ago) 1:52 PM

to Dale Curtis, Chromium Metrics Reviews, Sun Yueru, Chromium LUCI CQ, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Sun Yueru

Sangbaek Park added 1 comment

File tools/metrics/ukm/ukm.xml

Line 14192, Patchset 4 (Latest): <aggregation>

Sun Yueru . unresolved

This is for making this field available on go/ukm-dash, I suspect you don't need that since none of the other fields are aggregated, and I recall seeing a custom (Plx?) dash from Dale for viewing media metrics.
Otherwise LGTM.

Sangbaek Park

You're right. It's not aggregated so it won't be showing on go/ukm-dash. This field is added for PLX script queries when needed.

Sangbaek Park

@yr...@chromium.org, Dale stampled so PTAL again. Thank you!

Sun Yueru

It's not aggregated
This field is added for PLX script queries when needed.
The plx script doesn't read from the aggregated table, does it? If no one actually needs the <aggregation>, let's remove it if it's not used. Having <aggregation> incurs additional processing costs for UKM pipelines.

Sangbaek Park

@dalec...@chromium.org, I copied this RendererType ukm field including <aggregation> from Media.WebMediaPlayerState. Do we need this <aggregation> since my PLX scripts use `processed_chrome_ukm.readable_ukm.last7days` only? Or maybe some video stack team's PLX or ukm dashboard might use the aggregation in the future? If no, I'll remove the aggregation flag.

Related details

Attention is currently required from:

Sun Yueru

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dale Curtis (Gerrit)

unread,

2:51 PM (5 hours ago) 2:51 PM

to Sangbaek Park, Chromium Metrics Reviews, Sun Yueru, Chromium LUCI CQ, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Sangbaek Park

Dale Curtis added 1 comment

File tools/metrics/ukm/ukm.xml

Line 14192, Patchset 4 (Latest): <aggregation>

Sun Yueru . unresolved

This is for making this field available on go/ukm-dash, I suspect you don't need that since none of the other fields are aggregated, and I recall seeing a custom (Plx?) dash from Dale for viewing media metrics.
Otherwise LGTM.

Sangbaek Park

You're right. It's not aggregated so it won't be showing on go/ukm-dash. This field is added for PLX script queries when needed.

Sangbaek Park

@yr...@chromium.org, Dale stampled so PTAL again. Thank you!

Sun Yueru

It's not aggregated
This field is added for PLX script queries when needed.
The plx script doesn't read from the aggregated table, does it? If no one actually needs the <aggregation>, let's remove it if it's not used. Having <aggregation> incurs additional processing costs for UKM pipelines.

Sangbaek Park

@dalec...@chromium.org, I copied this RendererType ukm field including <aggregation> from Media.WebMediaPlayerState. Do we need this <aggregation> since my PLX scripts use `processed_chrome_ukm.readable_ukm.last7days` only? Or maybe some video stack team's PLX or ukm dashboard might use the aggregation in the future? If no, I'll remove the aggregation flag.

Dale Curtis

I don't think any of these fields work in the dashboard so it shouldn't be needed here. It'd be nice if we could use the default dashboard, but it's not expressive enough.

Related details

Attention is currently required from:

Sangbaek Park

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sangbaek Park (Gerrit)

unread,

4:21 PM (4 hours ago) 4:21 PM

to Dale Curtis, Chromium Metrics Reviews, Sun Yueru, Chromium LUCI CQ, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Dale Curtis and Sun Yueru

Sangbaek Park added 1 comment

File tools/metrics/ukm/ukm.xml

Line 14192, Patchset 4: <aggregation>

Sun Yueru . unresolved

This is for making this field available on go/ukm-dash, I suspect you don't need that since none of the other fields are aggregated, and I recall seeing a custom (Plx?) dash from Dale for viewing media metrics.
Otherwise LGTM.

Sangbaek Park

You're right. It's not aggregated so it won't be showing on go/ukm-dash. This field is added for PLX script queries when needed.

Sangbaek Park

@yr...@chromium.org, Dale stampled so PTAL again. Thank you!

Sun Yueru

It's not aggregated
This field is added for PLX script queries when needed.
The plx script doesn't read from the aggregated table, does it? If no one actually needs the <aggregation>, let's remove it if it's not used. Having <aggregation> incurs additional processing costs for UKM pipelines.

Sangbaek Park

@dalec...@chromium.org, I copied this RendererType ukm field including <aggregation> from Media.WebMediaPlayerState. Do we need this <aggregation> since my PLX scripts use `processed_chrome_ukm.readable_ukm.last7days` only? Or maybe some video stack team's PLX or ukm dashboard might use the aggregation in the future? If no, I'll remove the aggregation flag.

Dale Curtis

I don't think any of these fields work in the dashboard so it shouldn't be needed here. It'd be nice if we could use the default dashboard, but it's not expressive enough.

Sangbaek Park

Ack. I've removed <aggregation> from the newly added `RendererType`.
@yr...@chromium.org PTAL, thank you!

Related details

Attention is currently required from:

Dale Curtis
Sun Yueru

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sun Yueru (Gerrit)

unread,

4:44 PM (3 hours ago) 4:44 PM

to Sangbaek Park, Dale Curtis, Chromium Metrics Reviews, Chromium LUCI CQ, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Attention needed from Sangbaek Park

Sun Yueru voted Code-Review+1

Code-Review

+1

Related details

Attention is currently required from:

Sangbaek Park

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sangbaek Park (Gerrit)

unread,

5:05 PM (3 hours ago) 5:05 PM

to Sun Yueru, Dale Curtis, Chromium Metrics Reviews, Chromium LUCI CQ, Vikram Pasupathy, chromium...@chromium.org, asvitkine...@chromium.org, feature-me...@chromium.org

Sangbaek Park added 1 comment

File tools/metrics/ukm/ukm.xml

Line 14192, Patchset 4: <aggregation>

Sun Yueru . resolved

This is for making this field available on go/ukm-dash, I suspect you don't need that since none of the other fields are aggregated, and I recall seeing a custom (Plx?) dash from Dale for viewing media metrics.
Otherwise LGTM.

Sangbaek Park

You're right. It's not aggregated so it won't be showing on go/ukm-dash. This field is added for PLX script queries when needed.

Sangbaek Park

@yr...@chromium.org, Dale stampled so PTAL again. Thank you!

Sun Yueru

It's not aggregated
This field is added for PLX script queries when needed.
The plx script doesn't read from the aggregated table, does it? If no one actually needs the <aggregation>, let's remove it if it's not used. Having <aggregation> incurs additional processing costs for UKM pipelines.

Sangbaek Park

@dalec...@chromium.org, I copied this RendererType ukm field including <aggregation> from Media.WebMediaPlayerState. Do we need this <aggregation> since my PLX scripts use `processed_chrome_ukm.readable_ukm.last7days` only? Or maybe some video stack team's PLX or ukm dashboard might use the aggregation in the future? If no, I'll remove the aggregation flag.

Dale Curtis

I don't think any of these fields work in the dashboard so it shouldn't be needed here. It'd be nice if we could use the default dashboard, but it's not expressive enough.

Sangbaek Park

Ack. I've removed <aggregation> from the newly added `RendererType`.
@yr...@chromium.org PTAL, thank you!

Sangbaek Park

Done