[Webium] Skip CWV metrics for initial WebUI pages [chromium/src : main]
Ming-Ying Chung (Gerrit)
Ming-Ying Chung added 6 comments![]( "Open in Gerrit")
PTAL. This CL is now repurposed to rely on a shared base class `ukm_page_load_metrics_observer_base.{h,cc}`. The diff is better to read by comparing with PS#13, e.g. https://crrev.com/c/7763259/13..15
Let me know if there is any better approach, as the scale of this CL is quite large.
This CL adds an `is_webui_` flag to `UkmPageLoadMetricsObserver` and
conditionally skip recording these Core Web Vitals metrics for WebUI
pages, while still allowing the collection of other performance
metrics.
Ming-Ying ChungHmm I think this approach is still error prune of mixing these result...
How about creating webui_ukm_page_load_metrics_observer? And UkmPageLoadMetricsObserver should return `STOP_OBSERVING` when the URL is for InitialWebUI.
Introduced `UkmPageLoadMetricsObserverBase`.
bool is_webui_ = false;Ming-Ying ChungMaybe `is_initial_webui_` since this is not for generic WebUI?
removed.
if (waap::IsForInitialWebUI(navigation_handle->GetURL())) {Eriko KurimotoIn crrev.com/c/7760566, I see different logic to filter InitialWebUI URL out (`IsWebUISource()` in `chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc`).
Is there any reason why there are different? If no, which one is correct?
cc: @elk...@chromium.org
Ming-Ying ChungIsWebUISource is filtering WebUI, and IsForInitialWebUI is filtering Initial WebUI (e.g. WebUI toolbar).
In the page_load_metrics_browsertest.cc, the test expectes there is no separate loading process which ensures the metrics count, but if WebUI runs on the background the test would break. For now, Initial WebUI seems to be the only one which runs with simple url used in the test, but semantically filtering WebUI is correct.
But here, IIUC they want to filter InitialWebUI case only, right? Then IsForInitialWebUI looks better.
acked.
namespace ukm {
namespace builders {Please fix this WARNING reported by ClangTidy: check: modernize-concat-nested-namespaces
nested namespaces can be concatenated...
check: modernize-concat-nested-namespaces
nested namespaces can be concatenated (https://clang.llvm.org/extra/clang-tidy/checks/modernize/concat-nested-namespaces.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-concat-nested-namespaces` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
// Verify Kept metrics
tester()->test_ukm_recorder().ExpectEntryMetric(
entry, PageLoad::kPaintTiming_NavigationToFirstPaintName, 250);
tester()->test_ukm_recorder().ExpectEntryMetric(
entry, PageLoad::kPaintTiming_NavigationToFirstContentfulPaintName, 300);Ming-Ying ChungThis CL seems fine as a first fix, but I wonder if these might also affect some guardrail metrics. But we still want these UKMs for our performance analysis, so we can't just exclude them as well. Maybe eventually it's better to figure out how the UMA dashboard gets the data and if the dashboard can just exclude the initial WebUI data. Can you please add a TODO for that?
Now only include some non-CWV metrics for initial webui.
Maybe eventually it's better to figure out how the UMA dashboard gets the data and if the dashboard can just exclude the initial WebUI data. Can you please add a TODO for that.
Related details
- Annie Sullivan
- 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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
Hello,
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.
Related details
- Annie Sullivan
- 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 added 1 comment![]( "Open in Gerrit")
Hello,
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.
PTAL.
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.
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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 3 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.
PTAL.
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.
Thanks 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?
Do the updates to this file work without the other part of your change by any chance? Looks sweet!
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;
}I 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.cc
It turns out it works with gmock's negation matchers Not(HasMetric(...
Related details
- Annie Sullivan
- Ming-Ying Chung
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "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.
Thanks 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?
I 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.
Related details
- Annie Sullivan
- Ming-Ying Chung
- Rakina Zata Amni
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
Hi again ... I noticed that the page load metrics related integration test framework has also received updates related to logging webui related metrics in PageLoad events - filtering them for the tests, basically.
This is problematic because:
1) Querying TestUkmRecorder::GetMergedEntriesByName (e.g.) directly in a new test isn't covered by the filtering, so it will return UkmEntries for PageLoad events that don't have a source recorded. This now happens even for very simple tests, and is difficult to debug: Note that MetricIntegrationTest::IsWebUISource(nullptr) returns true - just now it took me a couple hours to notice that it's due to WebUI (there is really no hint, as there's no URL associated with the PageLoad events, not even chrome://).
2) It's not clear how webui related recording can be well tested, when the integration test framework filters the relevant events.