Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2514513004:
[BlobStorage] Fix UMA reporting. (Closed)
Description[BlobStorage] Fix UMA reporting.
R=jsbell@chromium.org
BUG=666862
Committed: https://crrev.com/1e85b53656cc7053706890de27b3061a1390a153
Cr-Commit-Position: refs/heads/master@{#433760}
Patch Set 1 #
Total comments: 1
Patch Set 2 : added CacheEntry to histograms.xml #
Messages
Total messages: 30 (18 generated)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hey Josh, Can you PTAL at this small uma fix patch? Daniel
Description was changed from ========== [BlobStorage] Fix UMA reporting. R=jsbell@chromium.org ========== to ========== [BlobStorage] Fix UMA reporting. R=jsbell@chromium.org B=666862 ==========
Description was changed from ========== [BlobStorage] Fix UMA reporting. R=jsbell@chromium.org B=666862 ========== to ========== [BlobStorage] Fix UMA reporting. R=jsbell@chromium.org BUG=666862 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2514513004/diff/1/storage/browser/blob/blob_s... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/2514513004/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_context.cc:316: UMA_HISTOGRAM_COUNTS("Storage.BlobItemSize.BlobSlice.CacheEntry", Per offline convo, this is missing in histograms.xml
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmurph@chromium.org changed reviewers: + mpearson@google.com
+mpearson for histograms change. This was supposed to be in there by whoever added the cache entry data backend to blobs. I've added the recording of the cache entry size for the BlobSlice, and we've already been recording it for BlobItemSize in general. The Bytes call was a mistake in my recent CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2016/11/19 00:10:49, dmurph wrote: > +mpearson for histograms change. > > This was supposed to be in there by whoever added the cache entry data backend > to blobs. I've added the recording of the cache entry size for the BlobSlice, > and we've already been recording it for BlobItemSize in general. > > The Bytes call was a mistake in my recent CL. Adding the new cache entry histogram looks fine. Removing the Storage.BlobItemSize.BlobSlice.Bytes does not look good, unless you can manage to get this patched in the same branch that introduced the logging bug. I think that means you need to merge this into M-56. If you can promise to do that, then I can approve this. --mark
On 2016/11/21 22:41:20, Mark P (out Nov 17-20) wrote: > On 2016/11/19 00:10:49, dmurph wrote: > > +mpearson for histograms change. > > > > This was supposed to be in there by whoever added the cache entry data backend > > to blobs. I've added the recording of the cache entry size for the BlobSlice, > > and we've already been recording it for BlobItemSize in general. > > > > The Bytes call was a mistake in my recent CL. > > Adding the new cache entry histogram looks fine. > > Removing the Storage.BlobItemSize.BlobSlice.Bytes does not look good, unless you > can manage to get this patched in the same branch that introduced the logging > bug. I think that means you need to merge this into M-56. If you can promise > to do that, then I can approve this. > > --mark Yes I definitely plan on merging this. Keeping that Bytes uma in there is very wrong and would mess up the stats.
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
lgtm
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479780136468910,
"parent_rev": "be9836d56caa5359e18945b4930e6f13c6b3a70a", "commit_rev":
"89108b5d33ba7ce35b66ea47914579ee6a9eebe8"}
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] Fix UMA reporting. R=jsbell@chromium.org BUG=666862 ========== to ========== [BlobStorage] Fix UMA reporting. R=jsbell@chromium.org BUG=666862 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] Fix UMA reporting. R=jsbell@chromium.org BUG=666862 ========== to ========== [BlobStorage] Fix UMA reporting. R=jsbell@chromium.org BUG=666862 Committed: https://crrev.com/1e85b53656cc7053706890de27b3061a1390a153 Cr-Commit-Position: refs/heads/master@{#433760} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1e85b53656cc7053706890de27b3061a1390a153 Cr-Commit-Position: refs/heads/master@{#433760} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
