New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 775321 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: 1
NextAction: ----
OS: Android
Pri: 2
Type: Bug


Show other hotlists

Hotlists containing this issue:
Hotlist-1


Sign in to add a comment

OfflinePages.ClearStoragePreRunUsage.* are being reported in bytes instead of MiB

Project Member Reported by carlosk@chromium.org, Oct 17 2017

Issue description

The reported values of the OfflinePages.ClearStoragePreRunUsage.* series of metrics are not being converted from the raw bytes values to MiB.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de426dcb636e12f2a90ea783cd8f42476473488e

commit de426dcb636e12f2a90ea783cd8f42476473488e
Author: Carlos Knippschild <carlosk@chromium.org>
Date: Thu Oct 19 20:34:33 2017

Fix reporting of OfflinePages.ClearStoragePreRunUsage.* metrics

Fixes the OfflinePages.ClearStoragePreRunUsage.* series of metrics --
renaming them to ClearStoragePreRunUsage2 -- so that they report their
values in KiB. They were previously expected to be reported in MiB but
were incorrectly reported in bytes.

Bug:  775321 
Change-Id: Ia12f0c039172292770ee066efc128b0fd9c8204e
Reviewed-on: https://chromium-review.googlesource.com/722033
Commit-Queue: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Peter Williamson <petewil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510191}
[modify] https://crrev.com/de426dcb636e12f2a90ea783cd8f42476473488e/components/offline_pages/core/offline_page_storage_manager.cc
[modify] https://crrev.com/de426dcb636e12f2a90ea783cd8f42476473488e/components/offline_pages/core/offline_page_storage_manager.h
[modify] https://crrev.com/de426dcb636e12f2a90ea783cd8f42476473488e/components/offline_pages/core/offline_page_storage_manager_unittest.cc
[modify] https://crrev.com/de426dcb636e12f2a90ea783cd8f42476473488e/tools/metrics/histograms/histograms.xml

Comment 2 by carl...@google.com, Oct 19 2017

dimich@ as you didn't participate in the CL review, would you please do a review pass of your own to comply with "Have the bug verified and assured that it's considered safe by other developers working in the same area". This is a step needed to allow merging to the beta branch.
Android Canary 64.0.3245.0 was released today and includes the CL from #c1. Starting the 24h wait time now.

Comment 4 by dim...@chromium.org, Oct 21 2017

I did review the CL in question as required by the process outlined in "M63 branch is here!" email sent to the team.

The change LGTM and seems to be small, local and free of issues. I think it is relatively safe to merge, if otherwise approved.

Labels: Merge-Request-63
I'd like to merge the change from #1 into the M63 branch. It has tests, has been doubly verified and has been deployed with Canary for more than 24h.

The histograms fixed by that change were found to be reported using the wrong unit what rendered their results practically useless. We intend to rely on them for monitoring disk space usage of two Offline Pages related features: Prefetching of Offline Pages (in experimentation, target launch of M63) and Offline Page Cache (launched; will be reviewed for landing). Without this fix there will be no direct measurement of that metric.
Project Member

Comment 6 by sheriffbot@chromium.org, Oct 24 2017

Labels: -Merge-Request-63 Hotlist-Merge-Approved Merge-Approved-63
Your change meets the bar and is auto-approved for M63. Please go ahead and merge the CL to branch 3239 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 24 2017

Labels: -merge-approved-63 merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/179b754f77b7007ce76f173cc43ed0b36e802524

commit 179b754f77b7007ce76f173cc43ed0b36e802524
Author: Carlos Knippschild <carlosk@chromium.org>
Date: Tue Oct 24 19:53:50 2017

Fix reporting of OfflinePages.ClearStoragePreRunUsage.* metrics (M63 merge)

Fixes the OfflinePages.ClearStoragePreRunUsage.* series of metrics --
renaming them to ClearStoragePreRunUsage2 -- so that they report their
values in KiB. They were previously expected to be reported in MiB but
were incorrectly reported in bytes.

Bug:  775321 
Change-Id: Ia12f0c039172292770ee066efc128b0fd9c8204e
Reviewed-on: https://chromium-review.googlesource.com/722033
Commit-Queue: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Peter Williamson <petewil@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#510191}(cherry picked from commit de426dcb636e12f2a90ea783cd8f42476473488e)
Reviewed-on: https://chromium-review.googlesource.com/736119
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#189}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/179b754f77b7007ce76f173cc43ed0b36e802524/components/offline_pages/core/offline_page_storage_manager.cc
[modify] https://crrev.com/179b754f77b7007ce76f173cc43ed0b36e802524/components/offline_pages/core/offline_page_storage_manager.h
[modify] https://crrev.com/179b754f77b7007ce76f173cc43ed0b36e802524/components/offline_pages/core/offline_page_storage_manager_unittest.cc
[modify] https://crrev.com/179b754f77b7007ce76f173cc43ed0b36e802524/tools/metrics/histograms/histograms.xml

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7169f118b1c26911a504e1746d33559c345e8045

commit 7169f118b1c26911a504e1746d33559c345e8045
Author: Carlos Knippschild <carlosk@chromium.org>
Date: Wed Oct 25 23:41:52 2017

Fix ClearStoragePreRunUsage histogram name

This histogram was renamed but the code reporting it was not updated
accordingly. This CL fixes that.

Bug:  775321 
Change-Id: I155239b3ccf187754c0c2536f6f44535ac3fe089
Reviewed-on: https://chromium-review.googlesource.com/737957
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Peter Williamson <petewil@chromium.org>
Commit-Queue: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511647}
[modify] https://crrev.com/7169f118b1c26911a504e1746d33559c345e8045/components/offline_pages/core/offline_page_storage_manager.cc
[modify] https://crrev.com/7169f118b1c26911a504e1746d33559c345e8045/components/offline_pages/core/offline_page_storage_manager_unittest.cc

Comment 9 by carl...@google.com, Oct 26 2017

Today we found a histogram naming bug in the initial CL that landed on master with as described in #8. A new beta merge will be needed. Waiting for it to land on Canary.

Comment 10 by carl...@google.com, Oct 26 2017

Change from #8 was released on Canary 64.0.3250.0 today. Waiting 24h.
Labels: Merge-Request-63
Requesting merge of the change from #8 to the M63 branch.
Labels: -Hotlist-Merge-Approved -merge-merged-3239
I guess I should remove the "merged" labels from before...
Project Member

Comment 13 by sheriffbot@chromium.org, Oct 27 2017

Labels: -Merge-Request-63 Hotlist-Merge-Reject Merge-Reject-63
The bug is marked as P3 or Feature. It should not be merged as M63 is in beta. 
Please contact the approriate milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Reject -Merge-Reject-63
Labels: -Pri-3 Merge-Request-63 Pri-2
Re-adding Merge-Request.
Project Member

Comment 16 by sheriffbot@chromium.org, Oct 31 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 17 Deleted

Approved the merge into M63 branch 3239. Please make sure to verify the fix after merging it.
Project Member

Comment 19 by bugdroid1@chromium.org, Nov 1 2017

Labels: -merge-approved-63 merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2433fadbad542d2b06873faa1639b0e1c2656b8

commit f2433fadbad542d2b06873faa1639b0e1c2656b8
Author: Carlos Knippschild <carlosk@chromium.org>
Date: Wed Nov 01 00:29:23 2017

Fix ClearStoragePreRunUsage histogram name (M63 merge)

This histogram was renamed but the code reporting it was not updated
accordingly. This CL fixes that.

Bug:  775321 
Change-Id: I155239b3ccf187754c0c2536f6f44535ac3fe089
Reviewed-on: https://chromium-review.googlesource.com/737957
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Peter Williamson <petewil@chromium.org>
Commit-Queue: Carlos Knippschild <carlosk@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#511647}(cherry picked from commit 7169f118b1c26911a504e1746d33559c345e8045)
Reviewed-on: https://chromium-review.googlesource.com/748281
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#330}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/f2433fadbad542d2b06873faa1639b0e1c2656b8/components/offline_pages/core/offline_page_storage_manager.cc
[modify] https://crrev.com/f2433fadbad542d2b06873faa1639b0e1c2656b8/components/offline_pages/core/offline_page_storage_manager_unittest.cc

Status: Verified (was: Assigned)
The fix has landed and has been merged. I also confirmed the histograms is working by looking at the dashboards.

Sign in to add a comment