Add trace event for complete network request

third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp

Index: third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
index 5105488d1075419512ef874a4c21adde757db359..30f608f21a77d6df7d2c0e3ec17d3fe3e1f19a06 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
@@ -42,6 +42,7 @@
 #include "platform/mhtml/MHTMLArchive.h"
 #include "platform/network/NetworkUtils.h"
 #include "platform/network/ResourceTimingInfo.h"
+#include "platform/tracing/NetworkInstrumentation.h"
 #include "platform/tracing/TraceEvent.h"
 #include "platform/tracing/TracedValue.h"
 #include "platform/weborigin/KnownPorts.h"
@@ -504,6 +505,8 @@ Resource* ResourceFetcher::requestResource(
     return nullptr;
 
   unsigned long identifier = createUniqueIdentifier();
+  network_instrumentation::beginResourceLoad(
+      identifier, request.url().getString().utf8().data());

[chiniforooshan, 2016/10/24 19:25:36]

Do you think we can move this (and the line before in which an id is created) to
the beginning of the function, right after the UMA update?

[caseq, 2016/10/24 22:47:46]

Do we really need it in the same place? I think it's important that it's _after_
the bail out -- otherwise we would have to take care of properly terminating the
event. I'd actually consider moving it down to actual start load, considering
the amount of bail outs here.

Also, let's pass blink strings as they are to the instrumentation call -- or
maybe just pass the entire request?

[Yoav Weiss, 2016/10/25 04:46:46]

I think it's important to be able to instrument this particular function, as
it's known to be fairly expensive.
Starting the instrumentation at the start load would ignore that.

[chiniforooshan, 2016/10/25 18:00:40]

Good point about terminating the event! Can we deal with that by making
BeginResourceLoad a scoped object that closes the event in its destructor if we
don't tell it otherwise, i.e. when a resource request is actually going to be
sent? IMO it's better to cover the logic in this function; people may be
specially interested in memcache stuff?

[dproy, 2016/11/03 15:13:37]

I added a scoped tracker. Does the overhead look reasonable?

   request.mutableResourceRequest().setPriority(computeLoadPriority(
       factory.type(), request, ResourcePriority::NotVisible));
   initializeResourceRequest(request.mutableResourceRequest(), factory.type(),
@@ -1119,6 +1122,7 @@ void ResourceFetcher::didFinishLoading(Resource* resource,
                                        int64_t encodedDataLength,
                                        DidFinishLoadingReason finishReason) {
   TRACE_EVENT_ASYNC_END0("blink.net", "Resource", resource->identifier());
+  network_instrumentation::endResourceLoad(resource->identifier());
   DCHECK(resource);
 
   // When loading a multipart resource, make the loader non-block when finishing
@@ -1169,6 +1173,7 @@ void ResourceFetcher::didFinishLoading(Resource* resource,
 void ResourceFetcher::didFailLoading(Resource* resource,
                                      const ResourceError& error) {
   TRACE_EVENT_ASYNC_END0("blink.net", "Resource", resource->identifier());
+  network_instrumentation::endResourceLoad(resource->identifier());

[caseq, 2016/10/24 22:47:46]

let's also pass success / fail flag down to the instrumentation method.

[dproy, 2016/11/03 15:13:37]

Done.

   removeResourceLoader(resource->loader());
   m_resourceTimingInfoMap.take(const_cast<Resource*>(resource));
   bool isInternalRequest = resource->options().initiatorInfo.name ==