Added support to listen on Network.responseReceived and expose that through tab.py

Added support to listen on Network.responseReceived and expose that through tab.py

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

[bolian, 2014-01-23 19:09:45]

This is the change we have talked about to support data reduction proxy tests's
need to inspect HTTP responses.

Please help add other reviewers if proper.

[tonyg, 2014-01-23 19:29:23]

This looks reasonable to me. My only comment is that we should add a unittest.

But since this involves modification of core APIs, either Dave or Nat should
review.

[bolian, 2014-01-24 01:31:53]

Thanks, added unit tests.

[bulach, 2014-01-24 05:23:05]

can't see patchset #6, says "invalid chunk", so commenting the previous one..
I have some suggestions below, it'll make this patch slightly more complicated
but will certainly simplify the following ones with the actual benchmarks...
please let me know your thoughts!

https://codereview.chromium.org/145923002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_network.py
(right):

https://codereview.chromium.org/145923002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:20: def
AddHTTPResponseListener(self, listener):
rather than expose the listener, would it be possible to have something more
like inspector_timeline.py?
i.e., this would have something like Start/StopMonitoringNetwork, and a model
which would parse / present the underlying data in a more pythonic format?

hopefully that wouldn't loose flexibility, and it'd make the tests simpler since
they won't need to know what a "listener" is, and how the format should be
parsed, etc... should the underlying devtools protocol ever change, we'd only
need to update at this level, rather than at the tests...

wdyt?

https://codereview.chromium.org/145923002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:51: def
_OnNotification(self, msg):
nit: _OnNetworkNotification?

https://codereview.chromium.org/145923002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:59:
traceback.print_exc()
just let it explode rather than swallowing the exception here?
but as above, hopefully we'll be able to get rid of the listeners, so rather
than calling an entirely unknown method here, we'd know exactly what exceptions
to catch..

[bolian, 2014-01-26 20:43:57]

Thanks for the review.
I changed the API. Something is wrong with my uploads since patch set 6 and I
also see "error: old chunk mismatch".  But if you click the Download [raw] link,
you can see the whole patch. I am not sure how to fix this. Maybe I will
sync/rebase and see.

https://codereview.chromium.org/145923002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_network.py
(right):

https://codereview.chromium.org/145923002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:20: def
AddHTTPResponseListener(self, listener):
Done. That sounds great. I'd like to follow current practice.

https://codereview.chromium.org/145923002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:51: def
_OnNotification(self, msg):
On 2014/01/24 05:23:06, bulach_ooo_until_feb_2 wrote:
> nit: _OnNetworkNotification?

Done.

https://codereview.chromium.org/145923002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:59:
traceback.print_exc()
Done. (Does not apply any more).

[bolian, 2014-01-28 20:20:49]

ping. Dave or Nat, could you take a look?

[nduca, 2014-01-29 01:09:20]

cant you do this from javascript and then build on that a WaitForSomething?

[bolian, 2014-01-29 02:25:43]

On 2014/01/29 01:09:20, nduca wrote:
> cant you do this from javascript and then build on that a WaitForSomething?

I don't quite follow.  Do you mean I add a document.readyState for this so that
the user of the API can wait for it?
Or, you mean I save the response in some javascript object?

[bolian, 2014-01-30 20:28:48]

ping. Nat, can you clarify the question?

[bulach, 2014-02-03 21:30:05]

thanks! just some drive by nits, otherwise I'm happy when nat / dave are happy..

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py
(right):

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py:358: def
GetRecordedHTTPResponse(self, url):
nit: how about url=None and remove the other one, or alternatively, rename this
to "ForURL" ?

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_network.py
(right):

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:9: raise
Exception("response must exist")
nit: instead of raising Exception, create an exception class here should anybody
ever wanted to capture it..
also, single quotes

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:22:
self._http_responses = {}
nit: collections.defaultdict(list)

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:56: def
GetRecordedHTTPResponse(self, url):
as above, to avoid the overload..

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:72:
self._http_responses[url].append(RecordedHTTPResponse(resp))
nit: and once this is a defaultdict, can remove the whole if block and just keep
this statement

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/backends/chrome/inspector_network_unittest.py
(right):

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network_unittest.py:31:
page = k[k.rfind("/") + 1 :]
nit: single quote

[bolian, 2014-02-04 02:05:29]

Thanks for the review!
I just realize that I will need the response body in some cases later, so I add
support for that in this cl.
I also updated the test case to cover more scenarios.

Sorry for adding this new change now, but I figure that this will save
everyone's time eventually.

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py
(right):

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py:358: def
GetRecordedHTTPResponse(self, url):
renamed to xxxForURL.

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_network.py
(right):

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:9: raise
Exception("response must exist")
On 2014/02/03 21:30:06, bulach wrote:
> nit: instead of raising Exception, create an exception class here should
anybody
> ever wanted to capture it..
> also, single quotes

Done.

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:22:
self._http_responses = {}
On 2014/02/03 21:30:06, bulach wrote:
> nit: collections.defaultdict(list)

Done.

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:56: def
GetRecordedHTTPResponse(self, url):
Done.

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:72:
self._http_responses[url].append(RecordedHTTPResponse(resp))
On 2014/02/03 21:30:06, bulach wrote:
> nit: and once this is a defaultdict, can remove the whole if block and just
keep
> this statement

Done.

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/backends/chrome/inspector_network_unittest.py
(right):

https://codereview.chromium.org/145923002/diff/440001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network_unittest.py:31:
page = k[k.rfind("/") + 1 :]
On 2014/02/03 21:30:06, bulach wrote:
> nit: single quote

Done.

[bolian, 2014-02-05 19:00:19]

ping?  Nat/Dave?

Nat, I am unclear what your suggestion about using javascript, could you
elaborate?

Thanks,
Bolian

[nduca, 2014-02-07 19:09:58]

You should figure out a way to do this without changing in telemetry, or explain
why it can't be done without a telemetry feature.

[bulach, 2014-02-07 19:45:37]

I can try to explain :)
this is a new "domain" in devtools protocol, not exposed via javascript.. it's
certainly doable to do this at the benchmark level rather than telemetry, but I
think telemetry is normally what speaks the devtools protocol, so this is the
"right" level?
if we were to do at the benchmark level, we'd have to expose the devtools
protocols, not sure if that's desirable...

[bolian, 2014-02-07 22:29:50]

Talking about this in an email thread.

[bolian, 2014-02-13 18:15:46]

Hi Tony,

Our email discussion leaves you as my reviewer. Did you get a chance to look
again?

[nduca, 2014-02-18 23:31:54]

tonyg: If we take this route, this should produce a timeline, ideally just be on
when timeline recording is on.

[bolian, 2014-02-25 03:15:35]

Hello,

As Nat suggested, I changed the CL to expose the recorded HTTP responses as a
timeline. I changed the timeline API so that there can be multiple named
timelines, the default one, confusingly named 'timeline', should keeps the same
behavior of the API as before.

The main reason I separate the network timeline out is that the new network
timeline is very heavy as it saves the response body. By default, the network
timeline won't be recorded, this could avoid huge additional overhead for other
measurements that don't need this. Another reason is that the current timeline
model has a freeze-after-import pattern, which I think is nice and like to keep.

I am not very familiar with the timeline API, please let me know what do you
think about such a change. I am happy do additional refactoring due to this if
needed.

Thanks,
Bolian

[nduca, 2014-02-25 04:52:47]

https://codereview.chromium.org/145923002/diff/700001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/web_contents.py (right):

https://codereview.chromium.org/145923002/diff/700001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/web_contents.py:91: def
StartTimelineRecording(self, timeline_names=None):
Promising start! I'd like to see one timeline created, rather than two. We're
trying to slowly generalize all our data systems in telemetry to produce a
unified timeline model, for various ONE RING TO RULE THEM ALL sort of thing. :)

Can we omit the full responses bodies from the events? If not, how about looking
into inspectors implementation (network_agent perhaps?) and see if you can make
it leaner. As I understand it, all your use case requires is the headers.

It seems like we should influence inspector protocol to make this more useful
for performance tracing than just take inspector as immutable.


Then, how about passing an options into the Start call saying what want to
enable? With about:tracing, we pass in a categories list, e.g.

StarTracing("-*,benchmarking,disabled-by-default-cc.debug") disables everything
but benchmarking and the "disabled-by-default-cc.deubg" category.

Maybe here TimelineRecordingOptions can be struct with just a field
   record_network=False
That a user can customize then pass into the StartTimelineRecording() method.

Wdyt?

[bolian, 2014-02-25 07:26:34]

https://codereview.chromium.org/145923002/diff/700001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/web_contents.py (right):

https://codereview.chromium.org/145923002/diff/700001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/web_contents.py:91: def
StartTimelineRecording(self, timeline_names=None):
Thanks for the review. Yes, I can combine the timelines into one.  I will change
TimelineModel so that it won't freeze after one import.

I actually do need the full response body, unfortunately. I will use the body to
compute the received content length (with possible gzipping) because 
Content-Length header is not reliable (and could be missing).  I am not quite
sure what you mean by making inspector implementation leaner.  I will have to
copy the body and keep it around for later inspection, right?  Alternatively, I
can request body on demand after events are recorded, but that will require more
API change (similar to my initial change).

I like to hear more about "we should influence inspector protocol to make this
more useful
for performance tracing than just take inspector as immutable".  Do you mean
inspector_timeline and inspector_network here?

I like the idea of TimelineRecordingOptions with a record_network field. That
seems all I need now.

I will work toward the above change.

[bolian, 2014-02-25 20:01:09]

I have updated the CL to combine the previous timeline timeline and network
timeline into one. The network timeline recording is by default off.

Please take another look, thanks!

[bolian, 2014-02-27 02:44:40]

On 2014/02/25 20:01:09, bolian wrote:
> I have updated the CL to combine the previous timeline timeline and network
> timeline into one. The network timeline recording is by default off.
> 
> Please take another look, thanks!

ping?

[tonyg, 2014-03-04 05:27:31]

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py
(right):

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py:45:
self._timeline_recording_options = None
Could we avoid stashing this object here by adding an is_started param to the
recorders. Then we can just stop everything that is started.

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py:213:
self._timeline_model.Freeze()
Can we do something where we call ImportTraces() for each timeline that is
recording here? Then Finalize the import once. That should avoid passing the
same timeline around.

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_network.py
(right):

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:6: from
cherrypy.lib import httputil as _httputil
I don't think we can add this dependency. Is it on chrome developers'
workstations by default? It isn't on mine.

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:15: class
RecordedHTTPResponse(object):
Seems like this should be an InspectorNetworkData akin to InspectorTimelineData.

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/timeline_recorder.py
(right):

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/timeline_recorder.py:5: class
TimelineRecorder(object):
How is this different from an Importer?

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/timeline/model.py (right):

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/timeline/model.py:25:
inspector_importer.InspectorTimelineImporter,
Let's add an inspector_importer.InspectorNetworkImporter that alleviates the
need for the freeze parameter.

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/timeline/thread.py (left):

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/timeline/thread.py:227: assert
len(self._toplevel_slices) == 0
Can you explain this?

[bolian, 2014-03-04 20:32:44]

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py
(right):

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py:45:
self._timeline_recording_options = None
Done.
I changed to use the existing _is_recording for this purpose and make Stop
returns the recorded events for model to import.
On 2014/03/04 05:27:31, tonyg wrote:
> Could we avoid stashing this object here by adding an is_started param to the
> recorders. Then we can just stop everything that is started.

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py:213:
self._timeline_model.Freeze()
Done.

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_network.py
(right):

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:6: from
cherrypy.lib import httputil as _httputil
Oh, I probably wrong. It is on my chromium + clank client by default, so I
thought it is available for all chrome devs by default. I will see if there is
other lib I can use. Let me know if you something in mind for this purpose. I
need this httputil here to normalize HTTP response headers (for example,
"content-type: text/html", will become "Content-Type: text/html").

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:15: class
RecordedHTTPResponse(object):
Hmm, this is only one of a few types of data received from the network domain
(Network.responseReceived). InspectorNetworkData seems too general if we add
other data types later.  Or, maybe I can change it to
InspectorNetworkHTTPReponseData?  what do you think?

On 2014/03/04 05:27:31, tonyg wrote:
> Seems like this should be an InspectorNetworkData akin to
InspectorTimelineData.

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/timeline_recorder.py
(right):

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/timeline_recorder.py:5: class
TimelineRecorder(object):
Sorry, the class doc was wrong and confusing. The TimelineRecorder will recorded
raw events and return them in Stop(), which will be imported by an Importer.
On 2014/03/04 05:27:31, tonyg wrote:
> How is this different from an Importer?

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/timeline/model.py (right):

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/timeline/model.py:25:
inspector_importer.InspectorTimelineImporter,
After moving model outside from timeline recorders and doing the import in
inspector_backend, I can keep the same freeze-on-init semantic as before and the
freeze parameter is now removed. I keep the network timeline events the same
structure as "timeline" timeline event, so they can use the same importer.

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/timeline/thread.py (left):

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/timeline/thread.py:227: assert
len(self._toplevel_slices) == 0
After moving model out of timeline recorder, I don't need this change any more.
The reason before was that I need to import events into model twice and the
assertion was not true for the second import.

[bolian, 2014-03-05 01:22:40]

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_network.py
(right):

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:6: from
cherrypy.lib import httputil as _httputil
Actually, all I need is camel case the header key. So I removed this dependency
and directly use title().

https://codereview.chromium.org/145923002/diff/750001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:15: class
RecordedHTTPResponse(object):
Now I see that the InspectorTimelineData recently added. I synced the change to
HEAD and merged with that change. In that context, this RecordedHTTPResponse
class corresponds to the "raw event" not the timeline data. I renamed the class
to be InspectorNetworkResponseData.

[tonyg, 2014-03-05 03:47:26]

lgtm

This is beautiful, thanks for bearing with us on the design.

[bolian, 2014-03-05 18:55:37]

The CQ bit was checked by bolian@chromium.org

[commit-bot: I haz the power, 2014-03-05 18:57:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bolian@chromium.org/145923002/810001

[commit-bot: I haz the power, 2014-03-05 19:23:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-05 19:23:27]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[tonyg, 2014-03-05 21:09:57]

The CQ bit was checked by tonyg@chromium.org

[commit-bot: I haz the power, 2014-03-05 21:10:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bolian@chromium.org/145923002/810001

[commit-bot: I haz the power, 2014-03-06 00:01:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bolian@chromium.org/145923002/810001

[commit-bot: I haz the power, 2014-03-06 01:19:50]

Change committed as 255200

[nduca, 2014-03-06 01:58:13]

this is really nice, awesomely done @bolian :)

[bolian, 2014-03-06 02:04:47]

Thanks for the suggestion to use Timeline! :)