Implement frame traversal for the navtiming and restiming plugins #9
Conversation
|
Thanks a lot. Will take me a bit to review, but your approach sounds right. |
|
Fyi, I've just updated the pull request with another change. I realised earlier that this sentence:
...was wrong, I hadn't considered nested frames at all in the first cut. The PR handles them now, they're implemented them in this commit if you want to look at it separately: https://github.com/philbooth/boomerang/commit/926d7f6fba859784fe17f8546a1f89bad6769279 Also, in case it's helpful, the little test-case I've been using is here: |
| } | ||
| } | ||
|
|
||
| return cachedFrames[url] = result; |
There was a problem hiding this comment.
Can you split this onto two lines so that it doesn't catch anyone by surprise?
|
Apart from my line notes, this looks good. |
|
Okey doke, updated with your feedback. |
| // nt_fet_st: 1403585426765, | ||
| // ..., | ||
| // frames: [ | ||
| // { |
There was a problem hiding this comment.
If the frame url makes it here, I'd add that as well.
|
Have pushed changes to use |
|
Since @philbooth has added everything necessary and according to Github this PR can be merged can we close this? Has this been reviewed to its fullest extend? |
|
I haven't been able to fully review it, but if you'd like to do so and post comments, that would help. |
|
So I guess one problem with this pull request is that it changes a lot of stuff, which introduces extra risk. The best way to mitigate that risk is, I reckon, to have a comprehensive unit test suite that covers current behaviour including all the edge cases and so on. This and other pull requests can then be verified against that suite and we can be more confident about not breaking stuff. Obviously, there is a lot of grunt work involved in writing a unit test suite, even more so for client-side code which has a ton of browser differences to work around. But I'm happy to help out on that front if you guys think it is a good way to go. Thoughts? |
|
I was thinking about unit-testing and best practices of implementing Unit-Tests in boomerang itself. I only hesitated yesterday to open up yet another issue just for an RFC on Unit-Testing or whats preferred by the users of Boomerang. What I can think of given that grunt has been integrated into boomerang on bluesmoon/boomerang I'd guess something like phantomjs or selenium and qunit or jasmine would be good. I worry however that not all parts of core boomerang.js can be tested this way as of now as they are not directly accessible. Splitting them up and building the closure for BOOMR upon calling An added benefit would be a better separation of concerns inside boomerang to make it easier for newcomers to look at and understand the code instead of having to grok through a large monolithic file. Does anyone of the people subscribed or watching this repository have pointers or suggestions for a good javascript Unit-Testing Framework? What we'd also need is the abillity to unit-test all the plugins since they do most of the actual work in terms of gathering the data. Therefore having an extensible suite of Unit-Tests that can be extended towards plugins would be excellent. @bluesmoon If I understood your history correctly you should've worked at yahoo around the time NCZ was there. Anything regarding his work on maintainable Javascript in production that you can remember? By the way the book that came out of his experience at yahoo is a tremendous resource. DISCLAIMER: I'm not affiliated with O'Reilly, NCZ or any of the book publishing involved parties, I just thought it was a really good book. |
|
Forked the unit-testing discussion to akamai#40, seemed like it warranted a separate thread. |
We need to skip the first item in the latency array before we do IQR filtering. The IQR filtering runs on a sorted array, which means we lose information about which item was first. This change drops that item before doing any other processing.
Sends plugins in the initial config.js request
|
Phil, I believe many of your ideas here are implemented in this SOASTA PR: akamai#68 It also includes a comprehensive unit and integration test suite. |
We need to skip the first item in the latency array before we do IQR filtering. The IQR filtering runs on a sorted array, which means we lose information about which item was first. This change drops that item before doing any other processing.
SPA test #9: AutoXHR After Load fix for IE Reviewed By: Charlie
The approach I've taken is to add a
framesarray property to the request when accessible frames are detected by either plugin. The format of each object inframesis similar to the main beacon format, only nested. So each object in the array has auproperty containing the URL and will also have properties from thenavtimingplugin, therestimingplugin or both. They may have nested frames too, which may have etc and so on.In doing this I made some changes to both plugins in order to get code-reuse. But essentially they are the same as they were before, with additional calls to some new functions that I added to the core module:
forEachFrameandaddFrameVar. Implementing those necessitated a further couple of private functions in the core module,addVarToandgetAccessibleFrames.Tested in latest Chrome, Firefox & IE.
What do you think, any good?