Shared Storage: Log fine-grained non-web-visible worklet error types [chromium/src : main]
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cammie Smith Barnes
| Commit-Queue | +1 |
1 comment
Related details
- Yao Xiao
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau removed dpr...@google.com from this change![]( "Open in Gerrit")
Related details
- Yao Xiao
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yao Xiao (Gerrit)
Yao Xiao voted and added 5 comments![]( "Open in Gerrit")
Votes added by Yao Xiao
| Code-Review | +1 |
5 comments
LGTM % comments
#include "base/containers/contains.h"Not needed if using equality checks (see the comment below).
if (base::Contains(error_message,
blink::kSharedStorageModuleScriptNotLoadedErrorMessage)) {Can we just check `if (error_message == blink::kSharedStorageModuleScriptNotLoadedErrorMessage) { ... }`. This should be faster.
Same below.
// for example,insert "OBSOLETE_" in the value's name after the inistial "k" toAdd a space after comma
<int value="1" label="AddModuleNonWebVisible">Per [this guideline](https://chromium.googlesource.com/chromium/src/tools/+/HEAD/metrics/histograms/README.md#remove-the-entry ), should this be updated to `(Obsolete) AddModuleNonWebVisible. Replaced by finer-grained types in 2024/04.`?
Same for `RunNonWebVisible` and `SelectURLNonWebVisible`.
Related details
- Cammie Smith Barnes
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 4 comments![]( "Open in Gerrit")
Not needed if using equality checks (see the comment below).
Done
if (base::Contains(error_message,
blink::kSharedStorageModuleScriptNotLoadedErrorMessage)) {Can we just check `if (error_message == blink::kSharedStorageModuleScriptNotLoadedErrorMessage) { ... }`. This should be faster.
Same below.
Done
// for example,insert "OBSOLETE_" in the value's name after the inistial "k" toAdd a space after comma
Done
Per [this guideline](https://chromium.googlesource.com/chromium/src/tools/+/HEAD/metrics/histograms/README.md#remove-the-entry ), should this be updated to `(Obsolete) AddModuleNonWebVisible. Replaced by finer-grained types in 2024/04.`?
Same for `RunNonWebVisible` and `SelectURLNonWebVisible`.
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cammie Smith Barnes
| Commit-Queue | +1 |
ayui@, PTAL at enums, thanks!
Related details
- Ayu Ishii
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 1 comment![]( "Open in Gerrit")
(Ignore compile failures on ios simulator for now, as it's a broader CQ issue, unrelated to this CL.)
Related details
- Ayu Ishii
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yao Xiao (Gerrit)
Yao Xiao voted and added 3 comments![]( "Open in Gerrit")
Votes added by Yao Xiao
| Code-Review | +1 |
3 comments
std::move(callback).Run(/*success=*/true, /*error_message=*/{});How about recording the `kSuccess` metric in the browser and not the renderer? That seems straightforward -- only 3 additions and 3 removals.
The flaw exists in today's recording anyway: "NonWebVisible" errors are captured in the browser before the renderer callback executes -- they can be recorded even if the callback does not invoke (e.g. racing with renderer shut down, etc). So, moving 'kSuccess' to the browser won't introduce new error types.
But if we double counting, it's a new type of error. The bucket proportions would all be wrong by a little / we would need to manually recalculate, which isn't great. WDYT?
Error in `run()` not visible to the document: croos-origin worklet operation"cross". Same for `SelectURLNonWebVisibleCrossOriginSharedStorageDisabled` below.
site..Remove the extra period. Same for `SelectURLNonWebVisibleCrossOriginSharedStorageDisabled` below.
Related details
- Ayu Ishii
- Cammie Smith Barnes
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes added 3 comments![]( "Open in Gerrit")
std::move(callback).Run(/*success=*/true, /*error_message=*/{});How about recording the `kSuccess` metric in the browser and not the renderer? That seems straightforward -- only 3 additions and 3 removals.
The flaw exists in today's recording anyway: "NonWebVisible" errors are captured in the browser before the renderer callback executes -- they can be recorded even if the callback does not invoke (e.g. racing with renderer shut down, etc). So, moving 'kSuccess' to the browser won't introduce new error types.
But if we double counting, it's a new type of error. The bucket proportions would all be wrong by a little / we would need to manually recalculate, which isn't great. WDYT?
Sounds good, will move kSuccess logging for the three operations in question to the browser.
I've decided to do that in a separate CL, so here I just add in the error types to the enum that I plan to use, so that the numbering is nice, and I update the TODOs to note which ones should go where.
(I discovered while writing tests that it was just a little fiddly, and it seems that it merits its own CL.)
Error in `run()` not visible to the document: croos-origin worklet operation"cross". Same for `SelectURLNonWebVisibleCrossOriginSharedStorageDisabled` below.
Done
Remove the extra period. Same for `SelectURLNonWebVisibleCrossOriginSharedStorageDisabled` below.
Done
Related details
- Ayu Ishii
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ayu Ishii (Gerrit)
Ayu Ishii voted and added 1 comment![]( "Open in Gerrit")
- Cammie Smith Barnes
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yao Xiao (Gerrit)
Yao Xiao voted Code-Review+1![]( "Open in Gerrit")
- Ayu Ishii
- Cammie Smith Barnes
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cammie Smith Barnes (Gerrit)
Cammie Smith Barnes voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Ayu Ishii
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Shared Storage: Log fine-grained non-web-visible worklet error types
Previously in shared storage worklet UMA metrics, we had broad error
buckets for the method and whether or not the error is web-visible.
The web-visible errors are something that developers can get feedback
on directly. But in a recent spike of non-web-visible errors, it would
have been nice to have finer-grained buckets to help see what was
going on. Here we split up the non-web-visible errors into more
specific buckets.
- M chrome/browser/storage/shared_storage_browsertest.cc
- M content/browser/shared_storage/shared_storage_document_service_impl.cc
- M content/browser/shared_storage/shared_storage_worklet_host.cc
- M third_party/blink/public/common/shared_storage/shared_storage_utils.h
- M third_party/blink/renderer/modules/shared_storage/shared_storage_worklet_global_scope.cc
- M tools/metrics/histograms/metadata/storage/enums.xml
Code-Review: +1 by Yao Xiao
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |