ServiceWorker: Add UMA for ServiceWorkerDatabase

ServiceWorker: Add UMA for ServiceWorkerDatabase


BUG=371675
TEST=n/a

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274793

[nhiroki, 2014-05-29 11:42:55]

PTAL, thanks!

[michaeln, 2014-05-29 23:46:27]

https://codereview.chromium.org/303483007/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/303483007/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:1011:
UMA_HISTOGRAM_ENUMERATION(kOperationErrorHistogramLabel,
I'm not sure how fine grained we want this to be?

Having the result of open be separate is definitely good and also reporting OK
events for that case is good so we can see rates.

Would it make sense to split ReadResults from WriteResult and to also log the OK
events so we can see rates?

https://codereview.chromium.org/303483007/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/303483007/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_database.h:42: enum Status {
please comment about these being used in uma so future changes should  append
only

[nhiroki, 2014-06-02 04:54:46]

Thank you for reviewing. Can you take another look?

To record read/write operation result separately, I revamped database operation
result handling.

Thanks!

https://codereview.chromium.org/303483007/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/303483007/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:1011:
UMA_HISTOGRAM_ENUMERATION(kOperationErrorHistogramLabel,
On 2014/05/29 23:46:28, michaeln wrote:
> I'm not sure how fine grained we want this to be?
> 
> Having the result of open be separate is definitely good and also reporting OK
> events for that case is good so we can see rates.
> 
> Would it make sense to split ReadResults from WriteResult and to also log the
OK
> events so we can see rates?

SGTM. The latest patchset records read/write results separately.

https://codereview.chromium.org/303483007/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/303483007/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_database.h:42: enum Status {
On 2014/05/29 23:46:28, michaeln wrote:
> please comment about these being used in uma so future changes should  append
> only

Done.

[michaeln, 2014-06-03 02:09:09]

lgtm

https://codereview.chromium.org/303483007/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (left):

https://codereview.chromium.org/303483007/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:994: void
ServiceWorkerDatabase::HandleError(
The new code looks correct. It maybe could have been accomplished with fewer
code changes approaching it from the point of view of refactoring this
HandleError instance method into HandleReadResult() HandleWriteResult() and
HandleOpenResult() and providing the OK results for logging to those methods as
well the errors. We generally prefer early return where possible instead and
instance methods like that which could do the disabling internally could have
helped with that. As coded more care has to be taken to call Disable()
throughout and that might be easy to miss.

https://codereview.chromium.org/303483007/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/303483007/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:86:
"ServiceWorker.Database.WriteResult";
I think this set of stats is good.

https://codereview.chromium.org/303483007/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:199:
kCurrentSchemaVersion < version) {
The last case may not necessarily mean corruption, say if somebody reverted to
an older version of chrome. I don't think actually happens very often, so i
think this is ok. That and i think it would be work to actually differentiate
between those two cases.

https://codereview.chromium.org/303483007/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:426:
DCHECK_EQ(STATUS_OK, status);
given line 424, not sure this dcheck is needed

[nhiroki, 2014-06-03 06:59:32]

Thank you for your comments. I replaced reporting functions with
Handle{Open,Read,Write}Results() as you commented. Can you take another look?

https://codereview.chromium.org/303483007/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (left):

https://codereview.chromium.org/303483007/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:994: void
ServiceWorkerDatabase::HandleError(
On 2014/06/03 02:09:09, michaeln wrote:
> The new code looks correct. It maybe could have been accomplished with fewer
> code changes approaching it from the point of view of refactoring this
> HandleError instance method into HandleReadResult() HandleWriteResult() and
> HandleOpenResult() and providing the OK results for logging to those methods
as
> well the errors.

SGTM! I'd prefer to work with Handle{...}Result() functions. Revamped.

> We generally prefer early return where possible instead and
> instance methods like that which could do the disabling internally could have
> helped with that. As coded more care has to be taken to call Disable()
> throughout and that might be easy to miss.

Agree. I think the latest patchset would be more secure.

https://codereview.chromium.org/303483007/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/303483007/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:199:
kCurrentSchemaVersion < version) {
On 2014/06/03 02:09:09, michaeln wrote:
> The last case may not necessarily mean corruption, say if somebody reverted to
> an older version of chrome. I don't think actually happens very often, so i
> think this is ok. That and i think it would be work to actually differentiate
> between those two cases.

Ah, good point! Added special logging for that case.

https://codereview.chromium.org/303483007/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:426:
DCHECK_EQ(STATUS_OK, status);
On 2014/06/03 02:09:09, michaeln wrote:
> given line 424, not sure this dcheck is needed

Removed them.

[nhiroki, 2014-06-03 07:04:55]

+asvitkine@, could you review histograms.xml as an OWNER? Thanks.

[michaeln, 2014-06-03 19:09:44]

lgtm, thnx!

[Alexei Svitkine (slow), 2014-06-03 19:16:35]

lgtm with optional nit

https://codereview.chromium.org/303483007/diff/190001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/303483007/diff/190001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:42220: +  <int value="0"
label="STATUS_OK"/>
Nit: These are meant to be human readable strings displayed on the dashboard, so
consider naming them differently - e.g. "Error not found".

[nhiroki, 2014-06-04 03:45:14]

Thank you!

https://codereview.chromium.org/303483007/diff/190001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/303483007/diff/190001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:42220: +  <int value="0"
label="STATUS_OK"/>
On 2014/06/03 19:16:36, Alexei Svitkine wrote:
> Nit: These are meant to be human readable strings displayed on the dashboard,
so
> consider naming them differently - e.g. "Error not found".

Done.

[nhiroki, 2014-06-04 03:45:18]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-06-04 03:46:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/303483007/230001

[commit-bot: I haz the power, 2014-06-04 06:17:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-06-04 12:36:55]

Change committed as 274793