[sub apps] add ukm metrics [chromium/src : main]
Vlad Krot (Gerrit)
Vlad Krot voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Luc Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 7 comments![]( "Open in Gerrit")
std::pair<blink::mojom::SubAppsServiceResultCode,Pairs are usually hard to read. Maybe define a struct for this?
void WebAppProvider::ReportSubAppMetricsOnStartup() {Maybe put this into DoDelayedPostStartupWork?
if (!profile_ || !content::AreIsolatedWebAppsEnabled(profile_)) {The profile_ check is redundant
if (!parent_app_id.has_value()) {This is redundant -- if the app matches the filter, the property is always correct
auto found = iwa_id_to_sub_apps_count.find(*parent_app_id);`iwa_id_to_sub_apps_count[*parent_app_id]++` works well, no?
for (auto& pair : iwa_id_to_sub_apps_count) {`for (const auto& [app_id, count] : ...)`
Related details
- Luc Nguyen
- Vlad Krot
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 3 comments![]( "Open in Gerrit")
ukm::builders::SubApp_CountPerParent(source_id)I don't think we need to defend against `kInvalidSourceId` here -- the call sites in the codebase that I see just inline this call
ukm::IwaSourceUrlRecorder::GetSourceIdForIwaUrl(app_origin.GetURL());ditto here
ukm::IwaSourceUrlRecorder::MarkSourceForDeletion(source_id);Why do we need to mark it for deletion every time?
Related details
- Luc Nguyen
- Vlad Krot
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. |
Vlad Krot (Gerrit)
Vlad Krot added 7 comments![]( "Open in Gerrit")
if (lock_) {How can it be null?
Good question. Considering the code above it cannot be null since it is not checked anywhere else.
Pairs are usually hard to read. Maybe define a struct for this?
Done
Maybe put this into DoDelayedPostStartupWork?
Done
if (!profile_ || !content::AreIsolatedWebAppsEnabled(profile_)) {The profile_ check is redundant
Done
This is redundant -- if the app matches the filter, the property is always correct
Done
auto found = iwa_id_to_sub_apps_count.find(*parent_app_id);`iwa_id_to_sub_apps_count[*parent_app_id]++` works well, no?
Yes, thanks for noting, I forgot that c++ has default values inserted on reads.
`for (const auto& [app_id, count] : ...)`
- Andrew Rayskiy
- Luc Nguyen
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. |
Vlad Krot (Gerrit)
Vlad Krot added 4 comments![]( "Open in Gerrit")
if (lock_) {Vlad KrotHow can it be null?
Good question. Considering the code above it cannot be null since it is not checked anywhere else.
Done
I don't think we need to defend against `kInvalidSourceId` here -- the call sites in the codebase that I see just inline this call
Done
ukm::IwaSourceUrlRecorder::GetSourceIdForIwaUrl(app_origin.GetURL());Vlad Krotditto here
Done
ukm::IwaSourceUrlRecorder::MarkSourceForDeletion(source_id);Why do we need to mark it for deletion every time?
This is how PWA also do it via `components/ukm/app_source_url_recorder.cc`.
For most metrics this source id auto deletes itself, but for IWA I made it to be like WebApp, manual deletion, so that we can create source id, send a few metrics and delete it manually. See `components/ukm/ukm_recorder_impl.cc` `MaybeMarkForDeletion()`
The source ids always have to be cleaned up from memory, the only question is to do it automatically or manually.
Related details
- Andrew Rayskiy
- Luc Nguyen
- Vlad Krot
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Shadowed: luke...@chromium.org
Reviewer source(s):
luke...@chromium.org is from context(googleclient/chrome/chromium_gwsq/chrome/browser/ui/web_applications/config.gwsq,googleclient/chrome/chromium_gwsq/chrome/browser/web_applications/config.gwsq)
Related details
- Alexei Svitkine
- Andrew Rayskiy
- Luc Nguyen
- Luke Deng
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 11 comments![]( "Open in Gerrit")
// For sub apps send UKM in addition to UMA that is send via callback chainingnitty-nit: `sent`
// Install dummy apps using isolated-app:// scheme URLs.
webapps::AppId parent_app_id =
test::InstallDummyWebApp(profile(), "Parent App", parent_url);
webapps::AppId app_id =
test::InstallDummyWebApp(profile(), "Sub App", sub_app_url);
// Make them IWAs and set parent relationship.
{
ScopedRegistryUpdate update = provider().sync_bridge_unsafe().BeginUpdate();
WebApp* app = update->UpdateApp(app_id);
app->SetParentAppId(parent_app_id);
WebApp* parent = update->UpdateApp(parent_app_id);
parent->SetIsolationData(
IsolationData::Builder(IwaStorageUnownedBundle(base::FilePath(
FILE_PATH_LITERAL("/dummy/path"))),
IwaVersion::Create("1.0.0").value())
.Build());
app->SetIsolationData(
IsolationData::Builder(IwaStorageUnownedBundle(base::FilePath(
FILE_PATH_LITERAL("/dummy/path2"))),
IwaVersion::Create("1.0.0").value())
.Build());
// Manually set pending update info.
proto::PendingUpdateInfo pending_update_info;
pending_update_info.set_name("New Name");
pending_update_info.set_was_ignored(false);
app->SetPendingUpdateInfo(std::move(pending_update_info));
}Will refer to @green...@google.com here on whether this works as a way to install IWAs, or if there are more robust ways to install IWAs.
static void ReportSubAppSilentUpdateResult(Just a heads up, this set of metrics will be triggered more often than the other ones. Commented in the UKM doc as well, the silent update mechanism triggers everytime a page visit happens to a single app, to check if there is a difference in the manifest w.r.t the existing app. If the manifest hasn't changed, the "flow" exits with ["AppUpToDate"](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/scheduler/manifest_silent_update_result.h;drc=a81bc8561d938a2cfa58705d6d7c1121a2dd0f78;l=23), so this metric might be more biased towards that.
Just something to keep an eye out.
base::flat_map<url::Origin, int> origin_to_subapps_count) {Since this is just a generic map, do we need to have it be sorted per `url::Origin`? It doesn't seem like we have to do it, in which case, can we use `absl::flat_hash_map` here as per [Chromium guidelines](https://source.chromium.org/chromium/chromium/src/+/main:base/containers/README.md;l=32-35?q=absl::flat_hash_map%20f:.md%20-f:gen%2F)?
const url::Origin& app_origin,nit: `parent_app_origin`?
.SetResult(static_cast<int64_t>(result))nitty-nit: Can we just use `int` here? The number of metrics shouldn't be enough to need 64 bits imo, wdyt?
TEST_F(IsolatedWebAppMetricsHelperTest, ReportNumInstalledSubApps) {This test is pretty good, but it only tests the metrics helper behavior (which forwards data to the UKM system). Instead, can we add the individual tests for these use-cases at the callsites, like we do for `chrome/browser/web_applications/commands/apply_pending_manifest_update_command_unittest.cc`? That allows us to test the whole behavior of the sub app system, instead of just verifying the UKM part of things.
If you want to keep this test around and add those E2E tests later on, that's fine too!
struct AddResultCodeWithMetric {
blink::mojom::SubAppsServiceResultCode result;
IsolatedWebAppMetricsHelper::LogSubAppInstallResult metric_result;
};
using AddResult =
base::expected<std::vector<blink::mojom::SubAppsServiceAddResultPtr>,
AddResultCodeWithMetric>;There's 3 different enums that are being sent to the UKM system to measure the output of one flow (whether the sub app was installed correctly, or if not, what happened). Can we streamline that behavior here, so that the flow returns a single enum metric for all the use-cases (both success and failure)? That makes this part more readable imo, wdyt?
AddCallInfo* add_call_info = base::FindOrNull(add_call_info_, add_call_id);Since `add_call_id` is only removed at the end of this function, `add_call_info` should always be present in the map here. Let's use `CHECK_DEREF(base::FindOrNull(...))` to explicitly state this invariant?
void ReportSubAppMetricsOnStartup();Let's make it `private:`?
for (const auto& [url, count] : iwa_id_to_sub_apps_count) {This isn't the url, but the `parent_app_id`, right? Let's rename this to `app_id`?
Related details
- Alexei Svitkine
- Andrew Rayskiy
- Luc Nguyen
- Vlad Krot
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. |
Vlad Krot (Gerrit)
Vlad Krot added 12 comments![]( "Open in Gerrit")
// For sub apps send UKM in addition to UMA that is send via callback chainingVlad Krotnitty-nit: `sent`
Done
// Install dummy apps using isolated-app:// scheme URLs.
webapps::AppId parent_app_id =
test::InstallDummyWebApp(profile(), "Parent App", parent_url);
webapps::AppId app_id =
test::InstallDummyWebApp(profile(), "Sub App", sub_app_url);
// Make them IWAs and set parent relationship.
{
ScopedRegistryUpdate update = provider().sync_bridge_unsafe().BeginUpdate();
WebApp* app = update->UpdateApp(app_id);
app->SetParentAppId(parent_app_id);
WebApp* parent = update->UpdateApp(parent_app_id);
parent->SetIsolationData(
IsolationData::Builder(IwaStorageUnownedBundle(base::FilePath(
FILE_PATH_LITERAL("/dummy/path"))),
IwaVersion::Create("1.0.0").value())
.Build());
app->SetIsolationData(
IsolationData::Builder(IwaStorageUnownedBundle(base::FilePath(
FILE_PATH_LITERAL("/dummy/path2"))),
IwaVersion::Create("1.0.0").value())
.Build());
// Manually set pending update info.
proto::PendingUpdateInfo pending_update_info;
pending_update_info.set_name("New Name");
pending_update_info.set_was_ignored(false);
app->SetPendingUpdateInfo(std::move(pending_update_info));
}Will refer to @green...@google.com here on whether this works as a way to install IWAs, or if there are more robust ways to install IWAs.
This is for sure not a robust way to install IWA, but I did this hacky way due to not knowing how to also install sub apps in unit tests. But you raised a valid concern, let me improve how it is done.
static void ReportSubAppSilentUpdateResult(Just a heads up, this set of metrics will be triggered more often than the other ones. Commented in the UKM doc as well, the silent update mechanism triggers everytime a page visit happens to a single app, to check if there is a difference in the manifest w.r.t the existing app. If the manifest hasn't changed, the "flow" exits with ["AppUpToDate"](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/scheduler/manifest_silent_update_result.h;drc=a81bc8561d938a2cfa58705d6d7c1121a2dd0f78;l=23), so this metric might be more biased towards that.
Just something to keep an eye out.
I guess it is fine, we would see that the command is called.
base::flat_map<url::Origin, int> origin_to_subapps_count) {Since this is just a generic map, do we need to have it be sorted per `url::Origin`? It doesn't seem like we have to do it, in which case, can we use `absl::flat_hash_map` here as per [Chromium guidelines](https://source.chromium.org/chromium/chromium/src/+/main:base/containers/README.md;l=32-35?q=absl::flat_hash_map%20f:.md%20-f:gen%2F)?
Done
const url::Origin& app_origin,Vlad Krotnit: `parent_app_origin`?
Done
ukm::IwaSourceUrlRecorder::MarkSourceForDeletion(source_id);Vlad KrotWhy do we need to mark it for deletion every time?
This is how PWA also do it via `components/ukm/app_source_url_recorder.cc`.
For most metrics this source id auto deletes itself, but for IWA I made it to be like WebApp, manual deletion, so that we can create source id, send a few metrics and delete it manually. See `components/ukm/ukm_recorder_impl.cc` `MaybeMarkForDeletion()`The source ids always have to be cleaned up from memory, the only question is to do it automatically or manually.
Acknowledged
nitty-nit: Can we just use `int` here? The number of metrics shouldn't be enough to need 64 bits imo, wdyt?
Right, let's change it.
TEST_F(IsolatedWebAppMetricsHelperTest, ReportNumInstalledSubApps) {This test is pretty good, but it only tests the metrics helper behavior (which forwards data to the UKM system). Instead, can we add the individual tests for these use-cases at the callsites, like we do for `chrome/browser/web_applications/commands/apply_pending_manifest_update_command_unittest.cc`? That allows us to test the whole behavior of the sub app system, instead of just verifying the UKM part of things.
If you want to keep this test around and add those E2E tests later on, that's fine too!
I propose a bit simpler solution. Move most of the logic except feature flag checking into `chrome/browser/web_applications/isolated_web_apps/isolated_web_app_metrics_helper.cc`, so that it can all be unit tested.
struct AddResultCodeWithMetric {
blink::mojom::SubAppsServiceResultCode result;
IsolatedWebAppMetricsHelper::LogSubAppInstallResult metric_result;
};
using AddResult =
base::expected<std::vector<blink::mojom::SubAppsServiceAddResultPtr>,
AddResultCodeWithMetric>;There's 3 different enums that are being sent to the UKM system to measure the output of one flow (whether the sub app was installed correctly, or if not, what happened). Can we streamline that behavior here, so that the flow returns a single enum metric for all the use-cases (both success and failure)? That makes this part more readable imo, wdyt?
I honestly do not see how it is feasible. For example we have `LogSubAppInstallResult::kSuccess` and `LogSubAppInstallResult::kSuccessBypassApproval`, due to this it is hard to streamline into single result of the flow because reporting of result and tracking state is in different places. And regardless of the result code, we send also metrics for individual results inside `std::vector<blink::mojom::SubAppsServiceAddResultPtr>`
AddCallInfo* add_call_info = base::FindOrNull(add_call_info_, add_call_id);Since `add_call_id` is only removed at the end of this function, `add_call_info` should always be present in the map here. Let's use `CHECK_DEREF(base::FindOrNull(...))` to explicitly state this invariant?
Done
void ReportSubAppMetricsOnStartup();Let's make it `private:`?
I also had such a thought, but the class has a lot of functions and no private modifier anywhere, so maybe for consistency leave as it is.
for (const auto& [url, count] : iwa_id_to_sub_apps_count) {This isn't the url, but the `parent_app_id`, right? Let's rename this to `app_id`?
- Alexei Svitkine
- Andrew Rayskiy
- Dibyajyoti Pal
- Luc Nguyen
Alexei Svitkine (Gerrit)
Alexei Svitkine voted Code-Review+1![]( "Open in Gerrit")
- Andrew Rayskiy
- Dibyajyoti Pal
- Luc Nguyen
- Vlad Krot
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. |
Luc Nguyen (Gerrit)
Luc Nguyen voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Andrew Rayskiy
- Dibyajyoti Pal
- Vlad Krot
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted and added 2 comments![]( "Open in Gerrit")
Votes added by Andrew Rayskiy
| Code-Review | +1 |
2 comments
"+absl/container/flat_hash_map.h",This looks out of place! The correct include would be `third_party/abseil-cpp/absl/container/flat_hash_map.h`, and it doesn't require an include rule IIUC
Are you still using it?
WebAppProvider* provider = GetWebAppProvider(render_frame_host());For the follow-up (if you don't mind): this class is extremely polluted with similar `GetWebAppProvider(render_frame_host())` calls everywhere. Can we turn this into a simple class function like `provider()` and inline it everywhere?
Related details
- Dibyajyoti Pal
- Vlad Krot
Vlad Krot (Gerrit)
Vlad Krot added 2 comments![]( "Open in Gerrit")
"+absl/container/flat_hash_map.h",This looks out of place! The correct include would be `third_party/abseil-cpp/absl/container/flat_hash_map.h`, and it doesn't require an include rule IIUC
Are you still using it?
Without this I get presubmit error
```
** Presubmit ERRORS: 1 **
You added one or more #includes that violate checkdeps rules.
chrome/browser/web_applications/isolated_web_apps/isolated_web_app_metrics_helper.cc
Illegal include: "absl/container/flat_hash_map.h"
Because of no rule applying.
```
WebAppProvider* provider = GetWebAppProvider(render_frame_host());For the follow-up (if you don't mind): this class is extremely polluted with similar `GetWebAppProvider(render_frame_host())` calls everywhere. Can we turn this into a simple class function like `provider()` and inline it everywhere?
- Andrew Rayskiy
- Dibyajyoti Pal
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Rayskiy
| Code-Review | +1 |
1 comment
"+absl/container/flat_hash_map.h",Vlad KrotThis looks out of place! The correct include would be `third_party/abseil-cpp/absl/container/flat_hash_map.h`, and it doesn't require an include rule IIUC
Are you still using it?
Without this I get presubmit error
```
** Presubmit ERRORS: 1 **
You added one or more #includes that violate checkdeps rules.
chrome/browser/web_applications/isolated_web_apps/isolated_web_app_metrics_helper.cc
Illegal include: "absl/container/flat_hash_map.h"
Because of no rule applying.
```
`absl/container/flat_hash_map.h` is an illegal include. Use `third_party/abseil-cpp/absl/container/flat_hash_map.h` and be happy?
Related details
- Dibyajyoti Pal
- Vlad Krot
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 5 comments![]( "Open in Gerrit")
This looks reasonably good, there's some places that can be improved imo, but nothing blocking for now.
static void ReportSubAppSilentUpdateResult(Vlad KrotJust a heads up, this set of metrics will be triggered more often than the other ones. Commented in the UKM doc as well, the silent update mechanism triggers everytime a page visit happens to a single app, to check if there is a difference in the manifest w.r.t the existing app. If the manifest hasn't changed, the "flow" exits with ["AppUpToDate"](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/scheduler/manifest_silent_update_result.h;drc=a81bc8561d938a2cfa58705d6d7c1121a2dd0f78;l=23), so this metric might be more biased towards that.
Just something to keep an eye out.
I guess it is fine, we would see that the command is called.
Acked, as long as you're aware of it that's fine.
CHECK(parent_app_id);Defensive programming, nice!
TEST_F(IsolatedWebAppMetricsHelperTest, ReportNumInstalledSubApps) {Vlad KrotThis test is pretty good, but it only tests the metrics helper behavior (which forwards data to the UKM system). Instead, can we add the individual tests for these use-cases at the callsites, like we do for `chrome/browser/web_applications/commands/apply_pending_manifest_update_command_unittest.cc`? That allows us to test the whole behavior of the sub app system, instead of just verifying the UKM part of things.
If you want to keep this test around and add those E2E tests later on, that's fine too!
I propose a bit simpler solution. Move most of the logic except feature flag checking into `chrome/browser/web_applications/isolated_web_apps/isolated_web_app_metrics_helper.cc`, so that it can all be unit tested.
This works for the use-case where we're measuring the sub apps and the count of the children (which kinda makes sense). How are you planning on moving the update specific use-cases to the IWA metrics helper?
struct AddResultCodeWithMetric {
blink::mojom::SubAppsServiceResultCode result;
IsolatedWebAppMetricsHelper::LogSubAppInstallResult metric_result;
};
using AddResult =
base::expected<std::vector<blink::mojom::SubAppsServiceAddResultPtr>,
AddResultCodeWithMetric>;Vlad KrotThere's 3 different enums that are being sent to the UKM system to measure the output of one flow (whether the sub app was installed correctly, or if not, what happened). Can we streamline that behavior here, so that the flow returns a single enum metric for all the use-cases (both success and failure)? That makes this part more readable imo, wdyt?
I honestly do not see how it is feasible. For example we have `LogSubAppInstallResult::kSuccess` and `LogSubAppInstallResult::kSuccessBypassApproval`, due to this it is hard to streamline into single result of the flow because reporting of result and tracking state is in different places. And regardless of the result code, we send also metrics for individual results inside `std::vector<blink::mojom::SubAppsServiceAddResultPtr>`
Actually, the example you shared is a good example where the result is already streamlined into a single enum to be measured.
What if we:
- Returned the result of all `Add` calls as `blink::mojom::SubAppsServiceResultCode` per call? We could update the enum values inside it as well to account for the cases where the bypass happened, instead of using a separate boolean value for it. We could store the result for it in `AddCallInfo` too.
- At the end of all the `Add` calls, update `ReportAddMetricsAndRunCallback()` to parse every `SubAppsServiceResultCode`, convert it to a `IsolatedWebAppMetricsHelper::LogSubAppInstallResult`, and send them to UKM.
Wdyt? Fine to do as a follow-up, the current code still seems complicated while parsing.
Related details
- Vlad Krot
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Vlad Krot
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
void ReportSubAppMetricsOnStartup();Vlad KrotLet's make it `private:`?
I also had such a thought, but the class has a lot of functions and no private modifier anywhere, so maybe for consistency leave as it is.
Acknowledged
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
"+absl/container/flat_hash_map.h",Vlad KrotThis looks out of place! The correct include would be `third_party/abseil-cpp/absl/container/flat_hash_map.h`, and it doesn't require an include rule IIUC
Are you still using it?
Andrew RayskiyWithout this I get presubmit error
```
** Presubmit ERRORS: 1 **
You added one or more #includes that violate checkdeps rules.
chrome/browser/web_applications/isolated_web_apps/isolated_web_app_metrics_helper.cc
Illegal include: "absl/container/flat_hash_map.h"
Because of no rule applying.
```
`absl/container/flat_hash_map.h` is an illegal include. Use `third_party/abseil-cpp/absl/container/flat_hash_map.h` and be happy?
- Andrew Rayskiy
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
struct AddResultCodeWithMetric {
blink::mojom::SubAppsServiceResultCode result;
IsolatedWebAppMetricsHelper::LogSubAppInstallResult metric_result;
};
using AddResult =
base::expected<std::vector<blink::mojom::SubAppsServiceAddResultPtr>,
AddResultCodeWithMetric>;Vlad KrotThere's 3 different enums that are being sent to the UKM system to measure the output of one flow (whether the sub app was installed correctly, or if not, what happened). Can we streamline that behavior here, so that the flow returns a single enum metric for all the use-cases (both success and failure)? That makes this part more readable imo, wdyt?
Dibyajyoti PalI honestly do not see how it is feasible. For example we have `LogSubAppInstallResult::kSuccess` and `LogSubAppInstallResult::kSuccessBypassApproval`, due to this it is hard to streamline into single result of the flow because reporting of result and tracking state is in different places. And regardless of the result code, we send also metrics for individual results inside `std::vector<blink::mojom::SubAppsServiceAddResultPtr>`
Actually, the example you shared is a good example where the result is already streamlined into a single enum to be measured.
What if we:
- Returned the result of all `Add` calls as `blink::mojom::SubAppsServiceResultCode` per call? We could update the enum values inside it as well to account for the cases where the bypass happened, instead of using a separate boolean value for it. We could store the result for it in `AddCallInfo` too.
- At the end of all the `Add` calls, update `ReportAddMetricsAndRunCallback()` to parse every `SubAppsServiceResultCode`, convert it to a `IsolatedWebAppMetricsHelper::LogSubAppInstallResult`, and send them to UKM.
Wdyt? Fine to do as a follow-up, the current code still seems complicated while parsing.
"We could update the enum values inside it as well to account for the cases where the bypass happened" - I actually was doing that before, but one of reviewers asked to not expose those states to blink side.
So we can have intermediate enum which later maps to `blink::mojom::SubAppsServiceResultCode` and `IsolatedWebAppMetricsHelper::LogSubAppInstallResult`
Related details
- Andrew Rayskiy
- Dibyajyoti Pal
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 3 comments![]( "Open in Gerrit")
"+absl/container/flat_hash_map.h",Vlad KrotThis looks out of place! The correct include would be `third_party/abseil-cpp/absl/container/flat_hash_map.h`, and it doesn't require an include rule IIUC
Are you still using it?
Andrew RayskiyWithout this I get presubmit error
```
** Presubmit ERRORS: 1 **
You added one or more #includes that violate checkdeps rules.
chrome/browser/web_applications/isolated_web_apps/isolated_web_app_metrics_helper.cc
Illegal include: "absl/container/flat_hash_map.h"
Because of no rule applying.
```
Vlad Krot`absl/container/flat_hash_map.h` is an illegal include. Use `third_party/abseil-cpp/absl/container/flat_hash_map.h` and be happy?
the second does not work, while the first works fine
I believe Andrew's suggestion is correct, we should be using that in the header files where we need, and not update the DEPS here. We already have usages of [`absl::flat_hash_map`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/model/web_app_icon_types.h;l=17?q=third_party%2Fabseil-cpp%2Fabsl%2Fcontainer%2Fflat_hash_map.h%20f:web_applications%2F) like that in our codebase, that should work.
url::Origin::Create(registrar.GetAppStartUrl(app_id_)), check_result);nit: `app_id_` here is the sub-app's ID. Since IWA sub-apps share the same origin as the parent IWA, `url::Origin::Create(...)` will evaluate to the parent app's origin, which practically matches what `ReportSubAppPendingUpdateResult` expects (`parent_app_origin`). However, consider explicitly fetching the parent app's start URL (via `parent_app_id()`) to align with the parameter name and improve readability.
struct AddResultCodeWithMetric {
blink::mojom::SubAppsServiceResultCode result;
IsolatedWebAppMetricsHelper::LogSubAppInstallResult metric_result;
};
using AddResult =
base::expected<std::vector<blink::mojom::SubAppsServiceAddResultPtr>,
AddResultCodeWithMetric>;Vlad KrotThere's 3 different enums that are being sent to the UKM system to measure the output of one flow (whether the sub app was installed correctly, or if not, what happened). Can we streamline that behavior here, so that the flow returns a single enum metric for all the use-cases (both success and failure)? That makes this part more readable imo, wdyt?
Dibyajyoti PalI honestly do not see how it is feasible. For example we have `LogSubAppInstallResult::kSuccess` and `LogSubAppInstallResult::kSuccessBypassApproval`, due to this it is hard to streamline into single result of the flow because reporting of result and tracking state is in different places. And regardless of the result code, we send also metrics for individual results inside `std::vector<blink::mojom::SubAppsServiceAddResultPtr>`
Vlad KrotActually, the example you shared is a good example where the result is already streamlined into a single enum to be measured.
What if we:
- Returned the result of all `Add` calls as `blink::mojom::SubAppsServiceResultCode` per call? We could update the enum values inside it as well to account for the cases where the bypass happened, instead of using a separate boolean value for it. We could store the result for it in `AddCallInfo` too.
- At the end of all the `Add` calls, update `ReportAddMetricsAndRunCallback()` to parse every `SubAppsServiceResultCode`, convert it to a `IsolatedWebAppMetricsHelper::LogSubAppInstallResult`, and send them to UKM.
Wdyt? Fine to do as a follow-up, the current code still seems complicated while parsing.
"We could update the enum values inside it as well to account for the cases where the bypass happened" - I actually was doing that before, but one of reviewers asked to not expose those states to blink side.
So we can have intermediate enum which later maps to `blink::mojom::SubAppsServiceResultCode` and `IsolatedWebAppMetricsHelper::LogSubAppInstallResult`
That would also work, as long as the code here is simplified to return a single thing. 😄
Related details
- Andrew Rayskiy
- Vlad Krot
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
"+absl/container/flat_hash_map.h",Vlad KrotThis looks out of place! The correct include would be `third_party/abseil-cpp/absl/container/flat_hash_map.h`, and it doesn't require an include rule IIUC
Are you still using it?
Andrew RayskiyWithout this I get presubmit error
```
** Presubmit ERRORS: 1 **
You added one or more #includes that violate checkdeps rules.
chrome/browser/web_applications/isolated_web_apps/isolated_web_app_metrics_helper.cc
Illegal include: "absl/container/flat_hash_map.h"
Because of no rule applying.
```
Vlad Krot`absl/container/flat_hash_map.h` is an illegal include. Use `third_party/abseil-cpp/absl/container/flat_hash_map.h` and be happy?
Dibyajyoti Palthe second does not work, while the first works fine
I believe Andrew's suggestion is correct, we should be using that in the header files where we need, and not update the DEPS here. We already have usages of [`absl::flat_hash_map`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/model/web_app_icon_types.h;l=17?q=third_party%2Fabseil-cpp%2Fabsl%2Fcontainer%2Fflat_hash_map.h%20f:web_applications%2F) like that in our codebase, that should work.
- Vlad Krot
Vlad Krot (Gerrit)
Vlad Krot added 4 comments![]( "Open in Gerrit")
"+absl/container/flat_hash_map.h",Vlad KrotThis looks out of place! The correct include would be `third_party/abseil-cpp/absl/container/flat_hash_map.h`, and it doesn't require an include rule IIUC
Are you still using it?
Andrew RayskiyWithout this I get presubmit error
```
** Presubmit ERRORS: 1 **
You added one or more #includes that violate checkdeps rules.
chrome/browser/web_applications/isolated_web_apps/isolated_web_app_metrics_helper.cc
Illegal include: "absl/container/flat_hash_map.h"
Because of no rule applying.
```
Vlad Krot`absl/container/flat_hash_map.h` is an illegal include. Use `third_party/abseil-cpp/absl/container/flat_hash_map.h` and be happy?
the second does not work, while the first works fine
sorry, you are right, I need to change c++ header and it works, great!
url::Origin::Create(registrar.GetAppStartUrl(app_id_)), check_result);nit: `app_id_` here is the sub-app's ID. Since IWA sub-apps share the same origin as the parent IWA, `url::Origin::Create(...)` will evaluate to the parent app's origin, which practically matches what `ReportSubAppPendingUpdateResult` expects (`parent_app_origin`). However, consider explicitly fetching the parent app's start URL (via `parent_app_id()`) to align with the parameter name and improve readability.
Done
TEST_F(IsolatedWebAppMetricsHelperTest, ReportNumInstalledSubApps) {Vlad KrotThis test is pretty good, but it only tests the metrics helper behavior (which forwards data to the UKM system). Instead, can we add the individual tests for these use-cases at the callsites, like we do for `chrome/browser/web_applications/commands/apply_pending_manifest_update_command_unittest.cc`? That allows us to test the whole behavior of the sub app system, instead of just verifying the UKM part of things.
If you want to keep this test around and add those E2E tests later on, that's fine too!
Dibyajyoti PalI propose a bit simpler solution. Move most of the logic except feature flag checking into `chrome/browser/web_applications/isolated_web_apps/isolated_web_app_metrics_helper.cc`, so that it can all be unit tested.
This works for the use-case where we're measuring the sub apps and the count of the children (which kinda makes sense). How are you planning on moving the update specific use-cases to the IWA metrics helper?
For `chrome/browser/web_applications/manifest_update_manager.cc` metrics I have added assertions of them being send to `chrome/browser/web_applications/sub_apps/sub_app_update_browsertest.cc`
Or do you prefer them being rather added to `ManifestSilentUpdateCommand`?
struct AddResultCodeWithMetric {
blink::mojom::SubAppsServiceResultCode result;
IsolatedWebAppMetricsHelper::LogSubAppInstallResult metric_result;
};
using AddResult =
base::expected<std::vector<blink::mojom::SubAppsServiceAddResultPtr>,
AddResultCodeWithMetric>;Vlad KrotThere's 3 different enums that are being sent to the UKM system to measure the output of one flow (whether the sub app was installed correctly, or if not, what happened). Can we streamline that behavior here, so that the flow returns a single enum metric for all the use-cases (both success and failure)? That makes this part more readable imo, wdyt?
Dibyajyoti PalI honestly do not see how it is feasible. For example we have `LogSubAppInstallResult::kSuccess` and `LogSubAppInstallResult::kSuccessBypassApproval`, due to this it is hard to streamline into single result of the flow because reporting of result and tracking state is in different places. And regardless of the result code, we send also metrics for individual results inside `std::vector<blink::mojom::SubAppsServiceAddResultPtr>`
Vlad KrotActually, the example you shared is a good example where the result is already streamlined into a single enum to be measured.
What if we:
- Returned the result of all `Add` calls as `blink::mojom::SubAppsServiceResultCode` per call? We could update the enum values inside it as well to account for the cases where the bypass happened, instead of using a separate boolean value for it. We could store the result for it in `AddCallInfo` too.
- At the end of all the `Add` calls, update `ReportAddMetricsAndRunCallback()` to parse every `SubAppsServiceResultCode`, convert it to a `IsolatedWebAppMetricsHelper::LogSubAppInstallResult`, and send them to UKM.
Wdyt? Fine to do as a follow-up, the current code still seems complicated while parsing.
Dibyajyoti Pal"We could update the enum values inside it as well to account for the cases where the bypass happened" - I actually was doing that before, but one of reviewers asked to not expose those states to blink side.
So we can have intermediate enum which later maps to `blink::mojom::SubAppsServiceResultCode` and `IsolatedWebAppMetricsHelper::LogSubAppInstallResult`
That would also work, as long as the code here is simplified to return a single thing. 😄
- Alexei Svitkine
- Andrew Rayskiy
- Dibyajyoti Pal
- Luc Nguyen
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal voted and added 3 comments![]( "Open in Gerrit")
Votes added by Dibyajyoti Pal
| Code-Review | +1 |
3 comments
LGTM, thanks for the tests and making the sub apps enums so simple! 😄
TEST_F(IsolatedWebAppMetricsHelperTest, ReportNumInstalledSubApps) {Vlad KrotThis test is pretty good, but it only tests the metrics helper behavior (which forwards data to the UKM system). Instead, can we add the individual tests for these use-cases at the callsites, like we do for `chrome/browser/web_applications/commands/apply_pending_manifest_update_command_unittest.cc`? That allows us to test the whole behavior of the sub app system, instead of just verifying the UKM part of things.
If you want to keep this test around and add those E2E tests later on, that's fine too!
Dibyajyoti PalI propose a bit simpler solution. Move most of the logic except feature flag checking into `chrome/browser/web_applications/isolated_web_apps/isolated_web_app_metrics_helper.cc`, so that it can all be unit tested.
Vlad KrotThis works for the use-case where we're measuring the sub apps and the count of the children (which kinda makes sense). How are you planning on moving the update specific use-cases to the IWA metrics helper?
For `chrome/browser/web_applications/manifest_update_manager.cc` metrics I have added assertions of them being send to `chrome/browser/web_applications/sub_apps/sub_app_update_browsertest.cc`
Or do you prefer them being rather added to `ManifestSilentUpdateCommand`?
Either of them is fine, I'll leave it to your discretion based on whichever is simpler :shrug:
The current tests added to `SubAppUpdateBrowserTest` LGTM!
// Internal enum to represent error codes of add function.
// It is remapped to ukm and mojo corresponding enums.
enum class AddCallErrorCode {
kUserDeclined,
kUserDeclinedEmbargo,
kLimitExceeded,
kWebAppsNotUserInstallable,
};Thanks for this, it's so much better and readable!
Related details
- Alexei Svitkine
- Andrew Rayskiy
- Vlad Krot
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. |