diff options
5 files changed, 222 insertions, 8 deletions
diff --git a/content/browser/service_worker/service_worker_metrics.cc b/content/browser/service_worker/service_worker_metrics.cc index c0d882d..0aaf7fa 100644 --- a/content/browser/service_worker/service_worker_metrics.cc +++ b/content/browser/service_worker/service_worker_metrics.cc @@ -116,4 +116,28 @@ void ServiceWorkerMetrics::RecordEventStatus(size_t fired_events, unhandled_ratio); } +void ServiceWorkerMetrics::RecordFetchEventStatus( + bool is_main_resource, + ServiceWorkerStatusCode status) { + if (is_main_resource) { + UMA_HISTOGRAM_ENUMERATION("ServiceWorker.FetchEvent.MainResource.Status", + status, SERVICE_WORKER_ERROR_MAX_VALUE); + } else { + UMA_HISTOGRAM_ENUMERATION("ServiceWorker.FetchEvent.Subresource.Status", + status, SERVICE_WORKER_ERROR_MAX_VALUE); + } +} + +void ServiceWorkerMetrics::RecordURLRequestJobResult( + bool is_main_resource, + URLRequestJobResult result) { + if (is_main_resource) { + UMA_HISTOGRAM_ENUMERATION("ServiceWorker.URLRequestJob.MainResource.Result", + result, NUM_REQUEST_JOB_RESULT_TYPES); + } else { + UMA_HISTOGRAM_ENUMERATION("ServiceWorker.URLRequestJob.Subresource.Result", + result, NUM_REQUEST_JOB_RESULT_TYPES); + } +} + } // namespace content diff --git a/content/browser/service_worker/service_worker_metrics.h b/content/browser/service_worker/service_worker_metrics.h index 400cd7e..fae19c4 100644 --- a/content/browser/service_worker/service_worker_metrics.h +++ b/content/browser/service_worker/service_worker_metrics.h @@ -35,6 +35,29 @@ class ServiceWorkerMetrics { NUM_DELETE_AND_START_OVER_RESULT_TYPES, }; + enum URLRequestJobResult { + REQUEST_JOB_FALLBACK_RESPONSE, + REQUEST_JOB_FALLBACK_FOR_CORS, + REQUEST_JOB_HEADERS_ONLY_RESPONSE, + REQUEST_JOB_STREAM_RESPONSE, + REQUEST_JOB_BLOB_RESPONSE, + REQUEST_JOB_ERROR_RESPONSE_STATUS_ZERO, + REQUEST_JOB_ERROR_BAD_BLOB, + REQUEST_JOB_ERROR_NO_PROVIDER_HOST, + REQUEST_JOB_ERROR_NO_ACTIVE_VERSION, + REQUEST_JOB_ERROR_NO_REQUEST, + REQUEST_JOB_ERROR_FETCH_EVENT_DISPATCH, + REQUEST_JOB_ERROR_BLOB_READ, + REQUEST_JOB_ERROR_STREAM_ABORTED, + REQUEST_JOB_ERROR_KILLED, + REQUEST_JOB_ERROR_KILLED_WITH_BLOB, + REQUEST_JOB_ERROR_KILLED_WITH_STREAM, + REQUEST_JOB_ERROR_DESTROYED, + REQUEST_JOB_ERROR_DESTROYED_WITH_BLOB, + REQUEST_JOB_ERROR_DESTROYED_WITH_STREAM, + NUM_REQUEST_JOB_RESULT_TYPES, + }; + // Used for ServiceWorkerDiskCache. static void CountInitDiskCacheResult(bool result); static void CountReadResponseResult(ReadResponseResult result); @@ -69,6 +92,15 @@ class ServiceWorkerMetrics { // the lifetime of ServiceWorker. static void RecordEventStatus(size_t fired_events, size_t handled_events); + // Records result of a ServiceWorkerURLRequestJob that was forwarded to + // the service worker. + static void RecordURLRequestJobResult(bool is_main_resource, + URLRequestJobResult result); + + // Records the result of dispatching a fetch event to a service worker. + static void RecordFetchEventStatus(bool is_main_resource, + ServiceWorkerStatusCode status); + private: DISALLOW_IMPLICIT_CONSTRUCTORS(ServiceWorkerMetrics); }; diff --git a/content/browser/service_worker/service_worker_url_request_job.cc b/content/browser/service_worker/service_worker_url_request_job.cc index efc871c..3a94d85 100644 --- a/content/browser/service_worker/service_worker_url_request_job.cc +++ b/content/browser/service_worker/service_worker_url_request_job.cc @@ -86,6 +86,16 @@ void ServiceWorkerURLRequestJob::Start() { } void ServiceWorkerURLRequestJob::Kill() { + if (ShouldRecordResult()) { + ServiceWorkerMetrics::URLRequestJobResult result = + ServiceWorkerMetrics::REQUEST_JOB_ERROR_KILLED; + if (response_body_type_ == STREAM) + result = ServiceWorkerMetrics::REQUEST_JOB_ERROR_KILLED_WITH_STREAM; + else if (response_body_type_ == BLOB) + result = ServiceWorkerMetrics::REQUEST_JOB_ERROR_KILLED_WITH_BLOB; + RecordResult(result); + } + net::URLRequestJob::Kill(); ClearStream(); fetch_dispatcher_.reset(); @@ -157,6 +167,7 @@ bool ServiceWorkerURLRequestJob::ReadRawData( return true; case Stream::STREAM_COMPLETE: DCHECK(!*bytes_read); + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_STREAM_RESPONSE); return true; case Stream::STREAM_EMPTY: stream_pending_buffer_ = buf; @@ -165,6 +176,7 @@ bool ServiceWorkerURLRequestJob::ReadRawData( return false; case Stream::STREAM_ABORTED: // Handle this as connection reset. + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_ERROR_STREAM_ABORTED); NotifyDone(net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_CONNECTION_RESET)); return false; @@ -182,9 +194,14 @@ bool ServiceWorkerURLRequestJob::ReadRawData( SetStatus(status); if (status.is_io_pending()) return false; + if (status.is_success() && *bytes_read == 0) + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_BLOB_RESPONSE); return status.is_success(); } +// TODO(falken): Refactor Blob and Stream specific handling to separate classes. +// Overrides for Blob reading ------------------------------------------------- + void ServiceWorkerURLRequestJob::OnReceivedRedirect( net::URLRequest* request, const net::RedirectInfo& redirect_info, @@ -227,14 +244,24 @@ void ServiceWorkerURLRequestJob::OnReadCompleted(net::URLRequest* request, int bytes_read) { SetStatus(request->status()); if (!request->status().is_success()) { + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_ERROR_BLOB_READ); NotifyDone(request->status()); return; } - NotifyReadComplete(bytes_read); - if (bytes_read == 0) + + if (bytes_read == 0) { + // Protect because NotifyReadComplete() can destroy |this|. + scoped_refptr<ServiceWorkerURLRequestJob> protect(this); + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_BLOB_RESPONSE); + NotifyReadComplete(bytes_read); NotifyDone(request->status()); + return; + } + NotifyReadComplete(bytes_read); } +// Overrides for Stream reading ----------------------------------------------- + void ServiceWorkerURLRequestJob::OnDataAvailable(Stream* stream) { // Clear the IO_PENDING status. SetStatus(net::URLRequestStatus()); @@ -255,12 +282,14 @@ void ServiceWorkerURLRequestJob::OnDataAvailable(Stream* stream) { case Stream::STREAM_COMPLETE: // Calling NotifyReadComplete with 0 signals completion. DCHECK(!bytes_read); + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_STREAM_RESPONSE); break; case Stream::STREAM_EMPTY: NOTREACHED(); break; case Stream::STREAM_ABORTED: // Handle this as connection reset. + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_ERROR_STREAM_ABORTED); NotifyDone(net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_CONNECTION_RESET)); break; @@ -283,6 +312,8 @@ void ServiceWorkerURLRequestJob::OnStreamRegistered(Stream* stream) { CommitResponseHeader(); } +// Misc ----------------------------------------------------------------------- + const net::HttpResponseInfo* ServiceWorkerURLRequestJob::http_info() const { if (!http_response_info_) return nullptr; @@ -319,6 +350,18 @@ void ServiceWorkerURLRequestJob::GetExtraResponseInfo( ServiceWorkerURLRequestJob::~ServiceWorkerURLRequestJob() { ClearStream(); + + if (!ShouldRecordResult()) + return; + // TODO(falken): If we don't see many of these, we might merge KILLED and + // DESTROYED results together. + ServiceWorkerMetrics::URLRequestJobResult result = + ServiceWorkerMetrics::REQUEST_JOB_ERROR_DESTROYED; + if (response_body_type_ == STREAM) + result = ServiceWorkerMetrics::REQUEST_JOB_ERROR_DESTROYED_WITH_STREAM; + else if (response_body_type_ == BLOB) + result = ServiceWorkerMetrics::REQUEST_JOB_ERROR_DESTROYED_WITH_BLOB; + RecordResult(result); } void ServiceWorkerURLRequestJob::MaybeStartRequest() { @@ -345,10 +388,17 @@ void ServiceWorkerURLRequestJob::StartRequest() { return; case FORWARD_TO_SERVICE_WORKER: - if (!provider_host_ || !provider_host_->active_version()) { + if (!provider_host_) { + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_ERROR_NO_PROVIDER_HOST); DeliverErrorResponse(); return; } + if (!provider_host_->active_version()) { + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_ERROR_NO_ACTIVE_VERSION); + DeliverErrorResponse(); + return; + } + DCHECK(!fetch_dispatcher_); // Send a fetch event to the ServiceWorker associated to the // provider_host. @@ -485,13 +535,16 @@ void ServiceWorkerURLRequestJob::DidDispatchFetchEvent( const ServiceWorkerResponse& response, scoped_refptr<ServiceWorkerVersion> version) { fetch_dispatcher_.reset(); + ServiceWorkerMetrics::RecordFetchEventStatus(is_main_resource_load_, status); // Check if we're not orphaned. - if (!request()) + if (!request()) { + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_ERROR_NO_REQUEST); return; + } if (status != SERVICE_WORKER_OK) { - // TODO(falken): Add UMA and the report error to the version. + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_ERROR_FETCH_EVENT_DISPATCH); if (is_main_resource_load_) { // Using the service worker failed, so fallback to network. Detach the // controller so subresource requests also skip the worker. @@ -512,6 +565,7 @@ void ServiceWorkerURLRequestJob::DidDispatchFetchEvent( if (request_mode_ == FETCH_REQUEST_MODE_CORS || request_mode_ == FETCH_REQUEST_MODE_CORS_WITH_FORCED_PREFLIGHT) { fall_back_required_ = true; + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_FALLBACK_FOR_CORS); CreateResponseHeader( 400, "Service Worker Fallback Required", ServiceWorkerHeaderMap()); CommitResponseHeader(); @@ -519,6 +573,7 @@ void ServiceWorkerURLRequestJob::DidDispatchFetchEvent( } // Change the response type and restart the request to fallback to // the network. + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_FALLBACK_RESPONSE); response_type_ = FALLBACK_TO_NETWORK; NotifyRestartRequired(); return; @@ -527,8 +582,11 @@ void ServiceWorkerURLRequestJob::DidDispatchFetchEvent( // We should have a response now. DCHECK_EQ(SERVICE_WORKER_FETCH_EVENT_RESULT_RESPONSE, fetch_result); - // Treat a response whose status is 0 as a Network Error. + // A response with status code 0 occurs when the respondWith promise was + // rejected or preventDefault() was called with no response provided. + // In this case, return a network error. if (response.status_code == 0) { + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_ERROR_RESPONSE_STATUS_ZERO); NotifyDone( net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED)); return; @@ -557,6 +615,7 @@ void ServiceWorkerURLRequestJob::DidDispatchFetchEvent( // Set up a request for reading the stream. if (response.stream_url.is_valid()) { DCHECK(response.blob_uuid.empty()); + SetResponseBodyType(STREAM); streaming_version_ = version; streaming_version_->AddStreamingURLRequestJob(this); response_url_ = response.url; @@ -579,12 +638,15 @@ void ServiceWorkerURLRequestJob::DidDispatchFetchEvent( CommitResponseHeader(); return; } + // Set up a request for reading the blob. if (!response.blob_uuid.empty() && blob_storage_context_) { + SetResponseBodyType(BLOB); scoped_ptr<storage::BlobDataHandle> blob_data_handle = blob_storage_context_->GetBlobDataFromUUID(response.blob_uuid); if (!blob_data_handle) { // The renderer gave us a bad blob UUID. + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_ERROR_BAD_BLOB); DeliverErrorResponse(); return; } @@ -598,8 +660,10 @@ void ServiceWorkerURLRequestJob::DidDispatchFetchEvent( CreateResponseHeader( response.status_code, response.status_text, response.headers); load_timing_info_.receive_headers_end = base::TimeTicks::Now(); - if (!blob_request_) + if (!blob_request_) { + RecordResult(ServiceWorkerMetrics::REQUEST_JOB_HEADERS_ONLY_RESPONSE); CommitResponseHeader(); + } } void ServiceWorkerURLRequestJob::CreateResponseHeader( @@ -641,6 +705,29 @@ void ServiceWorkerURLRequestJob::DeliverErrorResponse() { CommitResponseHeader(); } +void ServiceWorkerURLRequestJob::SetResponseBodyType(ResponseBodyType type) { + DCHECK_EQ(response_body_type_, UNKNOWN); + DCHECK_NE(type, UNKNOWN); + response_body_type_ = type; +} + +bool ServiceWorkerURLRequestJob::ShouldRecordResult() { + return !did_record_result_ && is_started_ && + response_type_ == FORWARD_TO_SERVICE_WORKER; +} + +void ServiceWorkerURLRequestJob::RecordResult( + ServiceWorkerMetrics::URLRequestJobResult result) { + DCHECK(ShouldRecordResult()); + // It violates style guidelines to handle a DCHECK fail condition but if there + // is a bug don't let it corrupt UMA results by double-counting. + if (!ShouldRecordResult()) + return; + did_record_result_ = true; + ServiceWorkerMetrics::RecordURLRequestJobResult(is_main_resource_load_, + result); +} + void ServiceWorkerURLRequestJob::ClearStream() { if (streaming_version_) { streaming_version_->RemoveStreamingURLRequestJob(this); diff --git a/content/browser/service_worker/service_worker_url_request_job.h b/content/browser/service_worker/service_worker_url_request_job.h index e29183a..bbae730 100644 --- a/content/browser/service_worker/service_worker_url_request_job.h +++ b/content/browser/service_worker/service_worker_url_request_job.h @@ -11,6 +11,7 @@ #include "base/memory/ref_counted.h" #include "base/memory/weak_ptr.h" #include "base/time/time.h" +#include "content/browser/service_worker/service_worker_metrics.h" #include "content/browser/streams/stream_read_observer.h" #include "content/browser/streams/stream_register_observer.h" #include "content/common/content_export.h" @@ -128,6 +129,12 @@ class CONTENT_EXPORT ServiceWorkerURLRequestJob FORWARD_TO_SERVICE_WORKER, }; + enum ResponseBodyType { + UNKNOWN, + BLOB, + STREAM, + }; + // We start processing the request if Start() is called AND response_type_ // is determined. void MaybeStartRequest(); @@ -160,6 +167,11 @@ class CONTENT_EXPORT ServiceWorkerURLRequestJob // Creates and commits a response header indicating error. void DeliverErrorResponse(); + // For UMA. + void SetResponseBodyType(ResponseBodyType type); + bool ShouldRecordResult(); + void RecordResult(ServiceWorkerMetrics::URLRequestJobResult result); + // Releases the resources for streaming. void ClearStream(); @@ -197,7 +209,7 @@ class CONTENT_EXPORT ServiceWorkerURLRequestJob FetchRequestMode request_mode_; FetchCredentialsMode credentials_mode_; - bool is_main_resource_load_; + const bool is_main_resource_load_; RequestContextType request_context_type_; RequestContextFrameType frame_type_; bool fall_back_required_; @@ -207,6 +219,9 @@ class CONTENT_EXPORT ServiceWorkerURLRequestJob scoped_ptr<storage::BlobDataHandle> request_body_blob_data_handle_; scoped_refptr<ServiceWorkerVersion> streaming_version_; + ResponseBodyType response_body_type_ = UNKNOWN; + bool did_record_result_ = false; + base::WeakPtrFactory<ServiceWorkerURLRequestJob> weak_factory_; DISALLOW_COPY_AND_ASSIGN(ServiceWorkerURLRequestJob); diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index a3f1573..6d75f41 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -36123,6 +36123,24 @@ Therefore, the affected-histogram name has to have at least one dot in it. </summary> </histogram> +<histogram name="ServiceWorker.FetchEvent.MainResource.Status" + enum="ServiceWorkerStatusCode"> + <owner>falken@chromium.org</owner> + <summary> + The result of dispatching a fetch event to a Service Worker for a main + resource request. + </summary> +</histogram> + +<histogram name="ServiceWorker.FetchEvent.Subresource.Status" + enum="ServiceWorkerStatusCode"> + <owner>falken@chromium.org</owner> + <summary> + The result of dispatching a fetch event to a Service Worker for a + subresource request. + </summary> +</histogram> + <histogram name="ServiceWorker.FetchEventExecutionTime" units="millisecond"> <owner>shimazu@chromium.org</owner> <summary> @@ -36269,6 +36287,22 @@ Therefore, the affected-histogram name has to have at least one dot in it. </summary> </histogram> +<histogram name="ServiceWorker.URLRequestJob.MainResource.Result" + enum="ServiceWorkerURLRequestJobResult"> + <owner>falken@chromium.org</owner> + <summary> + Records the result of a main resource request forwarded to a Service Worker. + </summary> +</histogram> + +<histogram name="ServiceWorker.URLRequestJob.Subresource.Result" + enum="ServiceWorkerURLRequestJobResult"> + <owner>falken@chromium.org</owner> + <summary> + Records the result of a subresource request forwarded to a Service Worker. + </summary> +</histogram> + <histogram name="ServiceWorkerCache.Cache" units="milliseconds"> <owner>dmurph@chromium.org</owner> <summary> @@ -63090,6 +63124,28 @@ To add a new entry, add it with any value and run test to compute valid value. <int value="17" label="SERVICE_WORKER_ERROR_REDUNDANT"/> </enum> +<enum name="ServiceWorkerURLRequestJobResult" type="int"> + <int value="0" label="REQUEST_JOB_FALLBACK_RESPONSE"/> + <int value="1" label="REQUEST_JOB_FALLBACK_FOR_CORS"/> + <int value="2" label="REQUEST_JOB_HEADERS_ONLY_RESPONSE"/> + <int value="3" label="REQUEST_JOB_STREAM_RESPONSE"/> + <int value="4" label="REQUEST_JOB_BLOB_RESPONSE"/> + <int value="5" label="REQUEST_JOB_ERROR_RESPONSE_STATUS_ZERO"/> + <int value="6" label="REQUEST_JOB_ERROR_BAD_BLOB"/> + <int value="7" label="REQUEST_JOB_ERROR_NO_PROVIDER_HOST"/> + <int value="8" label="REQUEST_JOB_ERROR_NO_ACTIVE_VERSION"/> + <int value="9" label="REQUEST_JOB_ERROR_NO_REQUEST"/> + <int value="10" label="REQUEST_JOB_ERROR_FETCH_EVENT_DISPATCH"/> + <int value="11" label="REQUEST_JOB_ERROR_BLOB_READ"/> + <int value="12" label="REQUEST_JOB_ERROR_STREAM_ABORTED"/> + <int value="13" label="REQUEST_JOB_ERROR_KILLED"/> + <int value="14" label="REQUEST_JOB_ERROR_KILLED_WITH_BLOB"/> + <int value="15" label="REQUEST_JOB_ERROR_KILLED_WITH_STREAM"/> + <int value="16" label="REQUEST_JOB_ERROR_DESTROYED"/> + <int value="17" label="REQUEST_JOB_ERROR_DESTROYED_WITH_BLOB"/> + <int value="18" label="REQUEST_JOB_ERROR_DESTROYED_WITH_STREAM"/> +</enum> + <enum name="ServiceWorkerWriteResponseResult" type="int"> <int value="0" label="OK"/> <int value="1" label="Write headers error"/> |