Add metric for new vs. existing H2/H3 sessions [chromium/src : main]
0 views
Skip to first unread message
Kenichi Ishibashi (Gerrit)
Sep 2, 2025, 3:18:36 AM (6 days ago) Sep 2
to Keita Suzuki, Takashi Toyoshima, Nidhi Jaju, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Attention needed from Keita Suzuki and Takashi Toyoshima
Kenichi Ishibashi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Commit-Queue | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 4 (Latest):
Kenichi Ishibashi . resolved
suzukikeita@: PTAL, does this look reasonable?
toyoshim@: PTAL histograms and mojom
Related details
Attention is currently required from:
- Keita Suzuki
- Takashi Toyoshima
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Sep 2, 2025, 5:16:10 AM (6 days ago) Sep 2
to Kenichi Ishibashi, Keita Suzuki, Nidhi Jaju, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Attention needed from Keita Suzuki and Kenichi Ishibashi
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
1 comment
File services/network/public/mojom/load_timing_internal_info.mojom
Line 9, Patchset 4 (Latest):
// Mirror of net::SessionSource.Takashi Toyoshima . unresolved
Just an idea, but how about adding triangle LINT.IfChange dependency loop among the //net, //services/network, and the enums.xml?
Currently, you set dependencies from //net to enums.xml, and from enums.xml to //net. But if we change the latter one from enums.xml to //services/network, and add one from //services/network to //net, we can write a dependency cycle without violating layering rules?
Related details
Attention is currently required from:
- Keita Suzuki
- Kenichi Ishibashi
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. |
Keita Suzuki (Gerrit)
Sep 2, 2025, 11:02:58 AM (6 days ago) Sep 2
to Kenichi Ishibashi, Takashi Toyoshima, Nidhi Jaju, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Attention needed from Kenichi Ishibashi
Keita Suzuki voted and added 2 comments![]( "Open in Gerrit")
Votes added by Keita Suzuki
| Code-Review | +1 |
2 comments
Patchset-level comments
Keita Suzuki . resolved
LGTM % comment. Thanks!
File tools/metrics/histograms/metadata/page/histograms.xml
Line 1654, Patchset 4 (Latest):
<token key="ConnectionProtocolType" variants="ConnectionProtocolType"/>Keita Suzuki . unresolved
Just want to make sure, with this we would be getting a histogram with ConnectionProtocolType being an empty string (i.e. `PageLoad.Clients.GoogleSearch.SessionSource`), but I assume we won't record in such case (also true for H1 and Other cases)?
Maybe it would be good to mention that we don't have any records for them since they would show up in the dashboard.
Related details
Attention is currently required from:
- Kenichi Ishibashi
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. |
Kenichi Ishibashi (Gerrit)
Sep 4, 2025, 10:21:41 PM (4 days ago) Sep 4
to Keita Suzuki, Takashi Toyoshima, Nidhi Jaju, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Attention needed from Takashi Toyoshima
Kenichi Ishibashi added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File services/network/public/mojom/load_timing_internal_info.mojom
Line 9, Patchset 4:
// Mirror of net::SessionSource.Takashi Toyoshima . unresolved
Just an idea, but how about adding triangle LINT.IfChange dependency loop among the //net, //services/network, and the enums.xml?
Currently, you set dependencies from //net to enums.xml, and from enums.xml to //net. But if we change the latter one from enums.xml to //services/network, and add one from //services/network to //net, we can write a dependency cycle without violating layering rules?
Kenichi Ishibashi
Good idea, updated. Did I do what you mean?
File tools/metrics/histograms/metadata/page/histograms.xml
Line 1654, Patchset 4:
<token key="ConnectionProtocolType" variants="ConnectionProtocolType"/>Keita Suzuki . resolved
Just want to make sure, with this we would be getting a histogram with ConnectionProtocolType being an empty string (i.e. `PageLoad.Clients.GoogleSearch.SessionSource`), but I assume we won't record in such case (also true for H1 and Other cases)?
Maybe it would be good to mention that we don't have any records for them since they would show up in the dashboard.
Kenichi Ishibashi
Yes, you're right. Updated to mention this is only available H2/H3.
Related details
Attention is currently required from:
- Takashi Toyoshima
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. |
Kenichi Ishibashi (Gerrit)
Sep 4, 2025, 10:26:50 PM (4 days ago) Sep 4
to Alex Moshchuk, Keita Suzuki, Takashi Toyoshima, Nidhi Jaju, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Attention needed from Alex Moshchuk and Takashi Toyoshima
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Attention is currently required from:
- Alex Moshchuk
- Takashi Toyoshima
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. |
Nidhi Jaju (Gerrit)
Sep 4, 2025, 10:42:50 PM (4 days ago) Sep 4
to Kenichi Ishibashi, Alex Moshchuk, Keita Suzuki, Takashi Toyoshima, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Attention needed from Alex Moshchuk, Keita Suzuki, Kenichi Ishibashi and Takashi Toyoshima
Nidhi Jaju voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nidhi Jaju
| Code-Review | +1 |
1 comment
File net/base/load_timing_internal_info.h
Line 28, Patchset 6 (Latest):
// LINT.ThenChange(//services/network/public/mojom/load_timing_internal_info.mojom:SessionSource)Nidhi Jaju . unresolved
I think it's better to keep this referring to enums.xml so we have the triangular dependency. In the current approach, if we change //net or //services/network, we might not change enums.xml.
```suggestion
// LINT.ThenChange(//tools/metrics/histograms/metadata/page/enums.xml:SessionSource)
```
Related details
Attention is currently required from:
- Alex Moshchuk
- Keita Suzuki
- Kenichi Ishibashi
- Takashi Toyoshima
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. |
Kenichi Ishibashi (Gerrit)
Sep 5, 2025, 12:59:59 AM (3 days ago) Sep 5
to Nidhi Jaju, Alex Moshchuk, Keita Suzuki, Takashi Toyoshima, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Attention needed from Alex Moshchuk, Keita Suzuki and Takashi Toyoshima
Kenichi Ishibashi added 2 comments![]( "Open in Gerrit")
File net/base/load_timing_internal_info.h
Line 28, Patchset 6:
// LINT.ThenChange(//services/network/public/mojom/load_timing_internal_info.mojom:SessionSource)Nidhi Jaju . resolved
I think it's better to keep this referring to enums.xml so we have the triangular dependency. In the current approach, if we change //net or //services/network, we might not change enums.xml.
```suggestion
// LINT.ThenChange(//tools/metrics/histograms/metadata/page/enums.xml:SessionSource)
```
Kenichi Ishibashi
Makes sense. Done.
Line 21, Patchset 6:
enum class SessionSource {Kenichi Ishibashi . unresolved
I thought it might be better to add `kUnspecified` and stop using `std::optional` for simple histogram and trace event recording, but I'm not fully sure which one is better.
Related details
Attention is currently required from:
- Alex Moshchuk
- Keita Suzuki
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Sep 5, 2025, 5:24:07 AM (3 days ago) Sep 5
to Kenichi Ishibashi, Nidhi Jaju, Alex Moshchuk, Keita Suzuki, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Attention needed from Alex Moshchuk, Keita Suzuki and Kenichi Ishibashi
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
1 comment
File services/network/public/mojom/load_timing_internal_info.mojom
Line 9, Patchset 4:
// Mirror of net::SessionSource.Takashi Toyoshima . resolved
Kenichi IshibashiJust an idea, but how about adding triangle LINT.IfChange dependency loop among the //net, //services/network, and the enums.xml?
Currently, you set dependencies from //net to enums.xml, and from enums.xml to //net. But if we change the latter one from enums.xml to //services/network, and add one from //services/network to //net, we can write a dependency cycle without violating layering rules?
Good idea, updated. Did I do what you mean?
Attention is currently required from:
- Alex Moshchuk
- Keita Suzuki
- Kenichi Ishibashi
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. |
Alex Moshchuk (Gerrit)
Sep 5, 2025, 1:41:50 PM (3 days ago) Sep 5
to Kenichi Ishibashi, Takashi Toyoshima, Nidhi Jaju, Keita Suzuki, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Attention needed from Keita Suzuki and Kenichi Ishibashi
Alex Moshchuk voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Keita Suzuki
- Kenichi Ishibashi
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. |
Kenichi Ishibashi (Gerrit)
Sep 5, 2025, 5:49:08 PM (3 days ago) Sep 5
to Alex Moshchuk, Takashi Toyoshima, Nidhi Jaju, Keita Suzuki, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Attention needed from Keita Suzuki
Kenichi Ishibashi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Commit-Queue | +2 |
1 comment
Patchset-level comments
Kenichi Ishibashi . resolved
Thank you.
Related details
Attention is currently required from:
- Keita Suzuki
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. |
Kenichi Ishibashi (Gerrit)
Sep 5, 2025, 5:51:05 PM (3 days ago) Sep 5
to Alex Moshchuk, Takashi Toyoshima, Nidhi Jaju, Keita Suzuki, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, Chromium LUCI CQ, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Attention needed from Keita Suzuki
Kenichi Ishibashi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Commit-Queue | +2 |
1 comment
File net/base/load_timing_internal_info.h
Line 21, Patchset 6:
enum class SessionSource {Kenichi Ishibashi . resolved
I thought it might be better to add `kUnspecified` and stop using `std::optional` for simple histogram and trace event recording, but I'm not fully sure which one is better.
Kenichi Ishibashi
Marked as resolved.
Related details
Attention is currently required from:
- Keita Suzuki
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Sep 5, 2025, 5:54:09 PM (3 days ago) Sep 5
to Kenichi Ishibashi, Alex Moshchuk, Takashi Toyoshima, Nidhi Jaju, Keita Suzuki, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Add metric for new vs. existing H2/H3 sessions
This change introduces a metric to track whether navigations to Google
search pages use a new or an existing HTTP/2 or HTTP/3 session.
This data will help in understanding connection reuse and performance
patterns for modern HTTP protocols.
A `SessionSource` enum is added to `net::LoadTimingInternalInfo` and
plumbed through the network service and content layers. This allows
the `GWSPageLoadMetricsObserver` to record the new
`PageLoad.Clients.GoogleSearch.SessionSource` histogram.
Bug: 441134585
Change-Id: I3a0ff4f053560af3b2afc327e8a8981b158c9e4b
Commit-Queue: Kenichi Ishibashi <ba...@chromium.org>
Reviewed-by: Takashi Toyoshima <toyo...@chromium.org>
Reviewed-by: Alex Moshchuk <ale...@chromium.org>
Reviewed-by: Nidhi Jaju <nidh...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1511887}
Files:
- M components/page_load_metrics/google/browser/gws_page_load_metrics_observer.cc
- M content/browser/renderer_host/navigation_request.cc
- M content/public/browser/navigation_handle_timing.h
- M net/base/load_timing_internal_info.h
- M services/network/public/cpp/load_timing_internal_info_mojom_traits.cc
- M services/network/public/cpp/load_timing_internal_info_mojom_traits.h
- M services/network/public/cpp/load_timing_internal_info_mojom_traits_unittest.cc
- M services/network/public/mojom/load_timing_internal_info.mojom
- M tools/metrics/histograms/metadata/page/enums.xml
- M tools/metrics/histograms/metadata/page/histograms.xml
Change size: M
Delta: 10 files changed, 106 insertions(+), 0 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Alex Moshchuk, +1 by Nidhi Jaju, +1 by Takashi Toyoshima
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chrome Crash (Prod) (Gerrit)
Sep 6, 2025, 5:20:30 AM (2 days ago) Sep 6
to Chromium LUCI CQ, Kenichi Ishibashi, Alex Moshchuk, Takashi Toyoshima, Nidhi Jaju, Keita Suzuki, chromium...@chromium.org, Chromium Metrics Reviews, AyeAye, ipc-securi...@chromium.org, speed-metrics...@chromium.org, loading-rev...@chromium.org, net-r...@chromium.org, csharris...@chromium.org, alexmo...@chromium.org, creis...@chromium.org, speed-metr...@chromium.org, network-ser...@chromium.org, asvitkine...@chromium.org, bmcquad...@chromium.org, navigation...@chromium.org, bnc+...@chromium.org
Chrome Crash (Prod) has created a revert of this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages