[webium] Isolate UKM metrics logging for initial WebUIs [chromium/src : main]
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 4 comments![]( "Open in Gerrit")
Ming-Ying ChungHello,
Based on crbug.com/490810407, I’m thinking the original objective was to record UKMs for initial webui URLs, which are always chrome:// URLs. This was achieved by enabling the UkmPageLoadMetricsObserver for these URLs, with the unfortunate side effect of diluting several important UMA histograms, including CLS - see crbug.com/502471507.
So I think the basic question is, which UKMs are actually required for the webui performance work?
And, is it possible to make a list of these, and record them separately (e.g. in InitialWebUIPageLoadMetricsObserver) to a new UKM event?
The resulting code and tests should be much easier to understand and maintain.
Please let me know what you think.
Johannes HenkelPTAL.
So I think the basic question is, which UKMs are actually required for the webui performance work?
I have made [this doc](https://docs.google.com/document/d/1ob71IMc-4TkEvAF3RBgziOmkS2CXn5RSHP5xdrD9f-I/edit?resourcekey=0-LXd9Ij064NcCaYkqbjaV2Q&tab=t.wpav6wc8d5wy) that lists all the existing metrics from `UkmPageLoadMetricsObserver` and discuss which ones we need for Initial WebUI.
And, is it possible to make a list of these, and record them separately (e.g. in InitialWebUIPageLoadMetricsObserver) to a new UKM event?
It's possible, but depending on how many metrics we log, there might be significant code duplication.
Shunya ShishidoThanks so much for making the doc - the detailed table looks great!
I think I'm noticing, there are metrics in ukm_page_load_metrics_observer_base.cc in this change (e.g. PageLoad.Net_HttpRttEstimate_OnNavigationStart) which are marked as 'skip' in your doc. This may be a good thing, right?
I think you're right about the code duplication, and there may be runtime cost and also, some maintenance cost when multiple locations need to be updated.
However, separation has its benefits as well, both for analyses (e.g. one can assume that PageLoad UKM events don't cover chrome:// URLs, like most existing colabs do) but also for maintenance, when making changes to either WebUI related metrics or traditional PageLoad metrics with the confidence that they won't interfere with each other. For instance, if I wanted to quickly log a metric to a UMA histogram that's in the superclass in your change (ukm_page_load_metrics_observer_base.cc), it may end up including the WebUI measurements accidentally. And vice versa, having your independent logging may allow you to move more quickly, in some cases.
What do you think?
Also, what are other people's opinions on this?
Ming-Ying ChungI also feel the current direction still has a risk to mix and pollute existing metrics. IMHO the priority is not to pollute the metrics rather than reducing the code duplication.
Repurposed again. Now no impact to existing non-initial webui ukm logging.
PTAL. I've repurposed this CL again. Only a few [selected ukm metrics](https://docs.google.com/document/d/1ob71IMc-4TkEvAF3RBgziOmkS2CXn5RSHP5xdrD9f-I/edit?resourcekey=0-LXd9Ij064NcCaYkqbjaV2Q&tab=t.wpav6wc8d5wy#heading=h.2lqx9cxe6uz) are logged for initial webui.
Ming-Ying ChungDo the updates to this file work without the other part of your change by any chance? Looks sweet!
reverted
MATCHER_P2(HasMetric, ukm_recorder, metric_name, "") {
if (!ukm_recorder->GetEntryMetric(arg, metric_name)) {
*result_listener << "where metric '" << metric_name << "' is missing";
return false;
}
return true;
}
MATCHER_P2(LacksMetric, ukm_recorder, metric_name, "") {
if (ukm_recorder->GetEntryMetric(arg, metric_name)) {
*result_listener << "where metric '" << metric_name
<< "' is unexpectedly present";
return false;
}
return true;
}Ming-Ying ChungI made something similar in spirit a while ago:
https://source.chromium.org/chromium/chromium/src/+/main:components/ukm/gmock_matchers.h
https://source.chromium.org/chromium/chromium/src/+/main:components/ukm/gmock_matchers_unittest.ccIt turns out it works with gmock's negation matchers Not(HasMetric(...
Reverted
Related details
- Annie Sullivan
- Johannes Henkel
- Rakina Zata Amni
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Annie Sullivan (Gerrit)
Annie Sullivan added 1 comment![]( "Open in Gerrit")
Thanks so much for making these changes! I'm happy with the new approach, but about to travel for a few days so delegating detailed OWNERS review to Johannes.
Related details
- Johannes Henkel
- Ming-Ying Chung
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
+1, thanks a lot for these updates! I like the structural changes a lot, so we may not have to worry about webui in the ukm_page_load_metrics_observer.cc any more; below two more points that would hopefully make us independent from another. :-)
---
In ukm_page_load_metrics_observer.cc (not yet in this changelist), I think it's still logging metrics that this change is now starting to log in initial_webui_page_load_metrics_observer.cc. Is that right? Do you want to remove this stanza from the ukm_page_load_metrics_observer.cc, in this change, so we don't double log things?
```
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
UkmPageLoadMetricsObserver::ShouldObserveScheme(const GURL& url) const {
#if !BUILDFLAG(IS_ANDROID)
if (waap::IsForInitialWebUI(url)) {
return CONTINUE_OBSERVING;
}
#endif
return page_load_metrics::PageLoadMetricsObserver::ShouldObserveScheme(url);
}
```
----
I think what I'm seeing in this change is that initial_webui_page_load_metrics_observer.cc is now logging to the PageLoad event.
That is, InitialWebUIPageLoadMetricsObserver::RecordPageLoadMetrics logs to ukm::builders::PageLoad.
Can you make a new event for this purpose please? As in, in ukm.xml, copy the PageLoad event so there is a webui specific event (Maybe InitialWebUiPageLoad or something like that), and it has the metrics that this changelist is logging?
I realize this would require an UKM collection review, but this should be quite easy because the metrics aren't new, they have been previously approved so you're just re-introducing them under a new name / new event. I had a similar situation recently, in which I added a bunch of metrics that were already logged to a new event and it was in fact a quick / easy review. If you'd like a pointer let me know.
Please let me know what you think!
Related details
- Ming-Ying Chung
- Rakina Zata Amni
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ming-Ying Chung
| Commit-Queue | +1 |
2 comments
+1, thanks a lot for these updates! I like the structural changes a lot, so we may not have to worry about webui in the ukm_page_load_metrics_observer.cc any more; below two more points that would hopefully make us independent from another. :-)
---
In ukm_page_load_metrics_observer.cc (not yet in this changelist), I think it's still logging metrics that this change is now starting to log in initial_webui_page_load_metrics_observer.cc. Is that right? Do you want to remove this stanza from the ukm_page_load_metrics_observer.cc, in this change, so we don't double log things?
```
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
UkmPageLoadMetricsObserver::ShouldObserveScheme(const GURL& url) const {
#if !BUILDFLAG(IS_ANDROID)
if (waap::IsForInitialWebUI(url)) {
return CONTINUE_OBSERVING;
}
#endif
return page_load_metrics::PageLoadMetricsObserver::ShouldObserveScheme(url);
}
```----
I think what I'm seeing in this change is that initial_webui_page_load_metrics_observer.cc is now logging to the PageLoad event.
That is, InitialWebUIPageLoadMetricsObserver::RecordPageLoadMetrics logs to ukm::builders::PageLoad.
Can you make a new event for this purpose please? As in, in ukm.xml, copy the PageLoad event so there is a webui specific event (Maybe InitialWebUiPageLoad or something like that), and it has the metrics that this changelist is logging?
I realize this would require an UKM collection review, but this should be quite easy because the metrics aren't new, they have been previously approved so you're just re-introducing them under a new name / new event. I had a similar situation recently, in which I added a bunch of metrics that were already logged to a new event and it was in fact a quick / easy review. If you'd like a pointer let me know.
Please let me know what you think!
Do you want to remove this stanza from the ukm_page_load_metrics_observer.cc, in this change, so we don't double log things?
Done.
> I think what I'm seeing in this change is that initial_webui_page_load_metrics_observer.cc is now logging to the PageLoad event.
Can you make a new event for this purpose please?
I have been thinking about these (and list potential new event names [in the doc](https://docs.google.com/document/d/1ob71IMc-4TkEvAF3RBgziOmkS2CXn5RSHP5xdrD9f-I/edit?resourcekey=0-LXd9Ij064NcCaYkqbjaV2Q&tab=t.wpav6wc8d5wy#heading=h.2lqx9cxe6uz). However, I am still not quite convinced to use new event names for initial webui:
- These are UKM and they are already sliced by URL.
- We are logging more than a single event, including PageLoad event and NavigationTiming, plus there are other UKM metrics not logged here, i.e. NavigationTimeline, are also logging initial webui UKM and we will also need them.
Given the above reasons, I didn't introduce new UKM events for them.
Related details
- Johannes Henkel
Annie Sullivan (Gerrit)
Annie Sullivan added 1 comment![]( "Open in Gerrit")
I'm sorry I missed this when I took a look this morning. It's very, very important not to make this change to the PageLoad event.
First, the PageLoad event is unsampled, and if I understand correctly this would create a very large amount of additional logs to it.
Second, over the years, there have been thousands of queries written and dozens of dashboards on the PageLoad event, and no doubt many of them assume that they're measuring web pages and not chrome:// URIs. I think it would be extremely difficult and time consuming to do a full audit, especially since many are likely peoples' personal scripts that wouldn't show up in searches.
Third, we do use the convention of logging a different UKM event for different types of page loads--HistoryNavigation, SoftNavigation, AmpPageLoad, PrerenderPageLoad. I think this is a similar case that should also have a separate event.
If you're really certain that UKM is the right direction for this patch, please use a new event. But also keep in mind that UMA is much easier to work with; it's automatically on dashboards for timelines and experiments with no custom queries required, and it doesn't need to be sampled.
Related details
- Johannes Henkel
- Ming-Ying Chung
- Rakina Zata Amni
- Shunya Shishido
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. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 1 comment![]( "Open in Gerrit")
ok that makes sense to me now.
Third, we do use the convention of logging a different UKM event for different types of page loads--HistoryNavigation, SoftNavigation, AmpPageLoad, PrerenderPageLoad. I think this is a similar case that should also have a separate event.
Are you also suggesting to not only introduce different UKM event for PageLoad, but also for other events like NavigationTiming and NavigationTimeline?
If you're really certain that UKM is the right direction for this patch, please use a new event. But also keep in mind that UMA is much easier to work with;
We already have UMAs. UKM will help use connect metrics within a single same session which is exactly what we need.
Related details
- Annie Sullivan
- Johannes Henkel
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 2 comments![]( "Open in Gerrit")
I had a similar situation recently, in which I added a bunch of metrics that were already logged to a new event and it was in fact a quick / easy review. If you'd like a pointer let me know.
I've filed UKM review at b/519396593. Please let me know if any way to speed it up. Thanks
Updated to log initial webui UKM metrics under `InitialWebUIPageLoad` and `InitialWebUINavigationTiming` instead of `PageLoad` and `NavigationTiming`. PTAL
Related details
- Annie Sullivan
- Johannes Henkel
- Rakina Zata Amni
- Shunya Shishido
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. |
Annie Sullivan (Gerrit)
Annie Sullivan added 1 comment![]( "Open in Gerrit")
Great! You could ask the owners of the `NavigationTimeline` events about whether they'd want this data, but I think having everything in your own event is the best thing to do. It makes the sampling consistent.
Also, for connecting metrics in the same session, note that Chrometto might also be very useful here!
Related details
- Johannes Henkel
- Ming-Ying Chung
- Rakina Zata Amni
- Shunya Shishido
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. |
Kamila Hasanbega (Gerrit)
Kamila Hasanbega added 1 comment![]( "Open in Gerrit")
<metric name="PageTiming.ForegroundDuration">(Passing by privacy ukm reviewer) The summary should give more information on what is being collected, at what granularity, when. Since most/all of the metrics here are dublicate metric then you could write a similar or same description as the original metric. For example here: https://source.chromium.org/chromium/chromium/src/+/main:tools/metrics/ukm/ukm.xml;l=17304?q=PageTiming.ForegroundDuration%20-f:out&ss=chromium#:~:text=17303-,17304,-17305
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 1 comment![]( "Open in Gerrit")
.InMilliseconds())Ming-Ying ChungWe need `navigation_commit_sent_time` too, to see if browser -> renderer commit ipc takes a long time. Also you might need to update your UKM collection review doc?
Done. added them back.
Related details
- Johannes Henkel
- Kamila Hasanbega
- Rakina Zata Amni
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted Code-Review+1![]( "Open in Gerrit")
- Johannes Henkel
- Kamila Hasanbega
- Ming-Ying Chung
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
yr...@chromium.org is from context(analysis/uma/chrome-metrics.gwsq)
Related details
- Johannes Henkel
- Kamila Hasanbega
- Shunya Shishido
- Sun Yueru
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 15 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Code-Review | +1 |
15 comments
Thanks a lot for the cleanups / removing unused things! I read through this once more, this time also the test and it looks nice.
My guess is the first two of my comments, what you have is probably intentional, so it's just a double check.
On the tests ... I think you can take advantage of the fact that you're introducing two new events, and that the initial_webui_page_load_metrics_observer.cc *never* records to the PageLoad or NavigationTiming events. So there's no need to make assertions about the other observer any more, or absence of CWV or similar.
class InitialWebUINavigationTiming;looks unused, maybe remove?
InitialWebUIPageLoadMetricsObserver::OnHidden(In UkmPageLoadMetricsObserver::OnHidden, sometimes recording of navigation & pageload metrics is triggered. But in this implementation, it's never triggered. Is this an intentional difference?
void InitialWebUIPageLoadMetricsObserver::RecordAbortMetrics(This does less than UkmPageLoadMetricsObserver::RecordAbortMetrics, specifically it's not recording PageVisitFinalStatus. Probably intentional?
If so, perhaps you could inline the body of this function into its only call-site for readability?
return recorder->EntryHasMetric(arg, metric_name);EntryHasMetric is a static method, so I think it's possible to write
TestUkmRecorder::EntryHasMetric(arg, metric_name);
Which means, the |recorder| argument to this custom matcher aren't needed, and all usages of the matcher in this file can be simplified.
The same is true for the matcher just below here, HasUkmMetricValue.
I wonder if these matchers are equivalent to the ones in components/ukm/gmock_matchers.h after you remove the |recorder| argument?
// UkmPageLoadMetricsObserver is NOT registered.No need to worry about that any more?
EXPECT_THAT(page_load_entries,
Each(Not(HasUkmMetric(
ukm_recorder_.get(),
"PaintTiming.NavigationToLargestContentfulPaint2"))));This part of the test isn't saying much, maybe remove.
EXPECT_THAT(entries,
Contains(HasUkmMetric(ukm_recorder_.get(),
"PaintTiming.NavigationToFirstPaint")));
EXPECT_THAT(entries, Contains(HasUkmMetric(
ukm_recorder_.get(),
"PaintTiming.NavigationToFirstContentfulPaint")));
}
// Verify PageLoad event is recorded with valid Document Timing.
IN_PROC_BROWSER_TEST_F(InitialWebUIPageLoadMetricsObserverBrowserTest,
VerifyDocumentTiming) {
GURL url(chrome::kChromeUIWebUIToolbarURL);
NavigateAndWaitForMetrics(url);
EXPECT_THAT(
GetEntriesForUrl("InitialWebUIPageLoad", url),
Contains(AllOf(
HasUkmMetric(ukm_recorder_.get(),
"DocumentTiming.NavigationToDOMContentLoadedEventFired"),
HasUkmMetric(ukm_recorder_.get(),
"DocumentTiming.NavigationToLoadEventFired"))));
}
// Verify PageLoad event is recorded with valid Parse Timing.
IN_PROC_BROWSER_TEST_F(InitialWebUIPageLoadMetricsObserverBrowserTest,
VerifyParseTiming) {
GURL url(chrome::kChromeUIWebUIToolbarURL);
NavigateAndWaitForMetrics(url);
EXPECT_THAT(GetEntriesForUrl("InitialWebUIPageLoad", url),
Contains(HasUkmMetric(ukm_recorder_.get(),
"ParseTiming.NavigationToParseStart")));I think these three tests list the 5 metrics that l. 282 is asserting?
Could this be just one test, maybe? It may be easier to read, if it says something like "we have 5 entries" and "the five entries are these 5"; it can still say in a comment that the first two are paint milestones, second two are document timing, then there's parse timing. Or similar.
// Verify PageLoad event does NOT contain any CWV metrics.I think this test is now unnecessary.
EXPECT_THAT(GetEntriesForUrl("PageLoad", url), IsEmpty());"InitialWebUIPageLoad"?
// If both standard and initial WebUI observers ran, we would have 2
// NavigationTiming entries. If only standard ran, we have 1.
EXPECT_THAT(GetEntriesForUrl("NavigationTiming", url), SizeIs(1));It may be better to assert that no InitialWebUINavigationTiming gets produced.
Asserting that some other observer produces entries, it makes the test brittle.
EXPECT_THAT(GetEntriesForUrl("PageLoad", url), SizeIs(8));I think this one too, it may be best to assert that no InitialWebUIPageLoad metrics are recorded for these pages, and leave it at that. Asserting it's 8 metrics for the ukm page load metrics observer makes this test brittle.
EXPECT_THAT(webui_entries,
Each(Not(HasUkmMetric(
ukm_recorder_.get(),
"PaintTiming.NavigationToLargestContentfulPaint2"))));Would avoid worrying about LCP in this test.
return recorder->EntryHasMetric(arg, metric_name);I commented in _browsertest.cc that this recorder argument may not be needed, since EntryHasMetric is a static method. Please look into it, also for the other matcher.
TEST_F(InitialWebUIPageLoadMetricsObserverTest, NoCWVMetrics) {
page_load_metrics::mojom::PageLoadTiming timing;
page_load_metrics::InitPageLoadTimingForTest(&timing);
timing.navigation_start = base::Time::FromSecondsSinceUnixEpoch(1);
timing.paint_timing->first_contentful_paint = base::Milliseconds(10);
timing.paint_timing->largest_contentful_paint->largest_text_paint =
base::Milliseconds(100);
timing.paint_timing->largest_contentful_paint->largest_text_paint_size = 20u;
timing.parse_timing->parse_start = base::Milliseconds(5);
PopulateRequiredTimingFields(&timing);
NavigateAndCommit(GURL(kTestWebUIUrl));
tester()->SimulateTimingUpdate(timing);
DeleteContents();
auto entries = test_ukm_recorder_.GetEntriesByName("InitialWebUIPageLoad");
EXPECT_THAT(entries, Not(IsEmpty()));
EXPECT_THAT(
entries,
Each(AllOf(
Not(HasUkmMetric(&test_ukm_recorder_,
"PaintTiming.NavigationToLargestContentfulPaint2")),
Not(HasUkmMetric(&test_ukm_recorder_,
"InteractiveTiming.FirstInputDelay4")),
Not(HasUkmMetric(&test_ukm_recorder_,
"LayoutInstability.MaxCumulativeShiftScore."
"SessionWindow.Gap1000ms"
".Max5000ms")))));
}
I think you can remove this test? InitiaLWebUIPageLoad can't have CWV, that's guaranteed by the UKM builders API.
Related details
- Kamila Hasanbega
- Ming-Ying Chung
- Shunya Shishido
- Sun Yueru
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Ming-Ying Chung (Gerrit)
Ming-Ying Chung voted and added 16 comments![]( "Open in Gerrit")
Votes added by Ming-Ying Chung
| Commit-Queue | +1 |
Thanks a lot for the cleanups / removing unused things! I read through this once more, this time also the test and it looks nice.
My guess is the first two of my comments, what you have is probably intentional, so it's just a double check.
On the tests ... I think you can take advantage of the fact that you're introducing two new events, and that the initial_webui_page_load_metrics_observer.cc *never* records to the PageLoad or NavigationTiming events. So there's no need to make assertions about the other observer any more, or absence of CWV or similar.
Thanks for careful review! I've addressed all of them and made the cc file aligned more with the original UkmPageLoadMetricsObserver if possible.
class InitialWebUINavigationTiming;Ming-Ying Chunglooks unused, maybe remove?
Done
In UkmPageLoadMetricsObserver::OnHidden, sometimes recording of navigation & pageload metrics is triggered. But in this implementation, it's never triggered. Is this an intentional difference?
Thanks for catching this. No I originally didn't think `OnHidden()`/`OnShown()` are related to the Initial WebUI measurement. However after manual testing they still look relevant (minimizing browser window on linux would trigger OnHidden() logging). Added the measurement back here to be aligned with UkmPageLoadMetricsObserver.
void InitialWebUIPageLoadMetricsObserver::RecordAbortMetrics(This does less than UkmPageLoadMetricsObserver::RecordAbortMetrics, specifically it's not recording PageVisitFinalStatus. Probably intentional?
If so, perhaps you could inline the body of this function into its only call-site for readability?
Yes this is intentional to skip the PageVisitFinalStatus UMAs.
I still want to keep this method to align with `UkmPageLoadMetricsObserver`
EntryHasMetric is a static method, so I think it's possible to write
TestUkmRecorder::EntryHasMetric(arg, metric_name);
Which means, the |recorder| argument to this custom matcher aren't needed, and all usages of the matcher in this file can be simplified.
The same is true for the matcher just below here, HasUkmMetricValue.
I wonder if these matchers are equivalent to the ones in components/ukm/gmock_matchers.h after you remove the |recorder| argument?
Thanks for the pointer. Switching to use `components/ukm/gmock_matchers.h`.
No need to worry about that any more?
Done
EXPECT_THAT(page_load_entries,
Each(Not(HasUkmMetric(
ukm_recorder_.get(),
"PaintTiming.NavigationToLargestContentfulPaint2"))));This part of the test isn't saying much, maybe remove.
Done
EXPECT_THAT(entries,I think these three tests list the 5 metrics that l. 282 is asserting?
Could this be just one test, maybe? It may be easier to read, if it says something like "we have 5 entries" and "the five entries are these 5"; it can still say in a comment that the first two are paint milestones, second two are document timing, then there's parse timing. Or similar.
Merged the first 2 but still make paint timing / document timing / parse timing separated.
// Verify PageLoad event does NOT contain any CWV metrics.I think this test is now unnecessary.
Removed
EXPECT_THAT(GetEntriesForUrl("PageLoad", url), IsEmpty());Ming-Ying Chung"InitialWebUIPageLoad"?
Done
// If both standard and initial WebUI observers ran, we would have 2
// NavigationTiming entries. If only standard ran, we have 1.
EXPECT_THAT(GetEntriesForUrl("NavigationTiming", url), SizeIs(1));It may be better to assert that no InitialWebUINavigationTiming gets produced.
Asserting that some other observer produces entries, it makes the test brittle.
Done
EXPECT_THAT(GetEntriesForUrl("PageLoad", url), SizeIs(8));I think this one too, it may be best to assert that no InitialWebUIPageLoad metrics are recorded for these pages, and leave it at that. Asserting it's 8 metrics for the ukm page load metrics observer makes this test brittle.
Done
EXPECT_THAT(webui_entries,
Each(Not(HasUkmMetric(
ukm_recorder_.get(),
"PaintTiming.NavigationToLargestContentfulPaint2"))));Would avoid worrying about LCP in this test.
Done
I commented in _browsertest.cc that this recorder argument may not be needed, since EntryHasMetric is a static method. Please look into it, also for the other matcher.
Done
TEST_F(InitialWebUIPageLoadMetricsObserverTest, NoCWVMetrics) {I think you can remove this test? InitiaLWebUIPageLoad can't have CWV, that's guaranteed by the UKM builders API.
- Jesse McKenna
- Johannes Henkel
- Kamila Hasanbega
- Rakina Zata Amni
- Shunya Shishido
- Sun Yueru
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ming-Ying Chung
| Commit-Queue | +1 |
1 comment
- Jesse McKenna
- Johannes Henkel
- Kamila Hasanbega
- Ming-Ying Chung
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Code-Review | +1 |
1 comment
Related details
- Jesse McKenna
- Kamila Hasanbega
- Ming-Ying Chung
- Rakina Zata Amni
- Shunya Shishido
- Sun Yueru
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Yueru (Gerrit)
Sun Yueru added 1 comment![]( "Open in Gerrit")
('InitialWebUIPageLoad', 'PageTiming.TotalForegroundDuration'),Could you fix the missing time units in the metric names instead of modifying this validation check?
Mark Pearson also pointed out some confusion in https://chromium-review.git.corp.google.com/c/chromium/src/+/7701615.
Related details
- Jesse McKenna
- Kamila Hasanbega
- Ming-Ying Chung
- Rakina Zata Amni
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Ming-Ying Chung (Gerrit)
Ming-Ying Chung voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ming-Ying Chung
| Commit-Queue | +1 |
2 comments
('InitialWebUIPageLoad', 'PageTiming.TotalForegroundDuration'),Could you fix the missing time units in the metric names instead of modifying this validation check?
Mark Pearson also pointed out some confusion in https://chromium-review.git.corp.google.com/c/chromium/src/+/7701615.
- Jesse McKenna
- Johannes Henkel
- Kamila Hasanbega
- Rakina Zata Amni
- Shunya Shishido
- Sun Yueru
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 1 comment![]( "Open in Gerrit")
Related details
- Jesse McKenna
- Johannes Henkel
- Kamila Hasanbega
- Rakina Zata Amni
- Shunya Shishido
- Sun Yueru
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ming-Ying Chung
| Commit-Queue | +1 |
1 comment
PTAL