Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add javascript timing metrics to timing panel onLoad if available #319

Merged
merged 5 commits into from Jun 21, 2013

Conversation

Projects
None yet
6 participants
Contributor

mindsocket commented Sep 6, 2012

Some browsers support navigation timing via javascript. Since these metrics fit the category of timing, this commit adds a section to the timing panel containing key metrics from the browser, triggered soon after the onLoad event.

The change is plain javascript, so no library dependencies.

Background:
https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html

Screenshot:
http://dl.dropbox.com/u/15940111/browser_timing.png

Tested on Chrome 22 and Firefox 14

SiDChik commented Oct 22, 2012

Nice feature, very usefull. Waiting for release:)

Contributor

mindsocket commented Nov 26, 2012

Is there anything I can do to help get this merged?

ebertti commented Feb 8, 2013

👍

Owner

jezdez commented Mar 2, 2013

I like the idea two, but can you do two things:

  • add a fallback for browsers that don't support it (which are those btw?)
  • afaik there are more than those 8 events that can be tracked, should DDT support more?
Contributor

mindsocket commented Mar 2, 2013

  • The javascript already includes that check (if (perf) {...} wraps most of the code in the commit)
  • I deliberately chose a subset of the timing events for 2 reasons...
    • Some are effectively duplicates (eg one event starts at the exact same time as another ends)
    • Some are not relevant to application debugging or performance in development (eg redirect time, domain lookup time)
Owner

jezdez commented Mar 3, 2013

Thanks @mindsocket, that sounds sensible. Regarding the fallback, I saw the condition check for perf but missed the .djDebugBrowserTiming block being hidden by default and only made visible if supported. Apologies.

With regard to the second point, I'm not sure if we can make an educated guess about all applications and which performance measuring is useful (e.g. domain lookup time isn't totally useless when debugging a frontend issue). Do you think we could make that configurable (without adding another setting)? Is there another way to specify all of them?

Contributor

mindsocket commented Mar 3, 2013

My admittedly opinionated approach was to choose metrics based on those
that are commonly relevant to debugging Django back- and front-end,
particularly at development time, rather than clutter up the timing data
for application developers with network- and DNS-related metrics (often a
production concern). It's still possible for developers to use the browser
console for the complete set. I'm not strongly of this opinion though, it
just seems a pragmatic one to me.

So, in terms of an alternative solution, one could simply add all of the
PerformanceTiming values listed at:
https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html

This appears to be possible without an explicit list by iterating over the
timing object in the Javascript block. Using Chrome console I put together
this proof-of-concept:
for (var metric in window.performance.timing) {
console.log(metric, window.performance.timing[metric]);
}

One way to make it "configurable" might be to have a basic list shown by
default, with an expandable section for the rest.

Let me know what you think, happy to let you run with it, or I can resubmit
w/ changes.

On 3 March 2013 22:33, Jannis Leidel notifications@github.com wrote:

Thanks @mindsocket https://github.com/mindsocket, that sounds sensible.
Regarding the fallback, I saw the condition check for perf but missed the
.djDebugBrowserTiming block being hidden by default and only made visible
if supported. Apologies.

With regard to the second point, I'm not sure if we can make an educated
guess about all applications and which performance measuring is useful
(e.g. domain lookup time isn't totally useless when debugging a frontend
issue). Do you think we could make that configurable (without adding
another setting)? Is there another way to specify all of them?


Reply to this email directly or view it on GitHubhttps://github.com/django-debug-toolbar/django-debug-toolbar/pull/319#issuecomment-14345584
.

Owner

jezdez commented Mar 3, 2013

You're right, network and DNS stats are less useful for most debugging sessions, but completely leaving them out would be a bad idea. When using the toolbar to get a picture of the performance of a Django site you usually try to get a picture of a bigger stack -- memory, database, cache, search index, etc. -- effectively the single reason why there are so different panels included. Getting access to information about browser performance is incredibly useful to eventually finding issues that other panels cannot describe successfully. I agree it may delude the frontend related performance stats, so maybe we should group the stats in "rendering" and "networking"?

Contributor

robhudson commented Mar 6, 2013

Very cool idea.

I'm curious... Are the timing set up calls initiated early enough? I haven't played with this yet but was curious.

Also, a waterfall like display would be slick but I wouldn't hold this back as it is.

Contributor

mindsocket commented Mar 6, 2013

@jezdez We could group them. I'll try to add the others in the coming days. IMHO the commit as is could go out without that in the interests of getting it in front of more eyes for feedback.

@robhudson The timing is captured automatically by the browser (it's a builtin in many modern browsers), the javascript above just grabs it when all is said and done (after page load).

A waterfall (a la SQL queries) is a great idea, if I get a chance I'll take a look

Contributor

mindsocket commented Mar 24, 2013

timing_sample
I've rebased my commit and rearranged things somewhat.

@jezdez I've included more of the timing events. Also, based on @robhudson 's suggestion I took inspiration from the SQL panel and rendered a timeline of timing events. I'm not sure I like the output exactly as I have it now, but it's an improvement.

Owner

jezdez commented Mar 24, 2013

Awesome, this looks amazing @mindsocket! Any feedback @robhudson?

Contributor

mindsocket commented Apr 30, 2013

Is there anything I can do to help get this merged?

Contributor

robhudson commented Apr 30, 2013

Apologies... I missed the original call from @jezdez. This looks pretty good.

My only concern is that this is using inline scripts. I've been away from debug toolbar but thought we had some efforts to reduce the amount of inline scripting. (I just found issue #330 which would actually be nice) Would it be much more effort to put this in the javascript file with a hook into the panel display events?

@robhudson robhudson commented on an outdated diff Apr 30, 2013

debug_toolbar/templates/debug_toolbar/panels/timer.html
+ <tbody id="djDebugBrowserTimingTableBody">
+ </tbody>
+ </table>
+</div>
+<script>
+ (function(w, d) {
+ function _renderPerf() {
+ // Browser timing is hidden unless we can successfully access the performance object
+ var perf = w.performance || w.msPerformance ||
+ w.webkitPerformance || w.mozPerformance;
+ if (perf) {
+ var rowCount = 0,
+ timingOffset = perf.timing.navigationStart,
+ timingEnd = perf.timing.loadEventEnd;
+ var totalTime = timingEnd - timingOffset;
+ console.log(timingOffset, timingEnd, totalTime);
@robhudson

robhudson Apr 30, 2013

Contributor

We might want this removed before merging.

@robhudson robhudson commented on an outdated diff Apr 30, 2013

debug_toolbar/templates/debug_toolbar/panels/timer.html
+ addRow('responseStart', 'responseEnd');
+ addRow('domLoading', 'domComplete'); // Spans the events below
+ addRow('domInteractive');
+ addRow('domContentLoadedEventStart', 'domContentLoadedEventEnd');
+ addRow('loadEventStart', 'loadEventEnd');
+ d.getElementById('djDebugBrowserTiming').style.display = "block";
+ }
+ }
+
+ function renderPerf() {
+ setTimeout(_renderPerf, 1000);
+ }
+
+ if (w.addEventListener) { w.addEventListener("load", renderPerf, false); }
+ else if (w.attachEvent) { w.attachEvent("onload", renderPerf); }
+ }(window, document));
@robhudson

robhudson Apr 30, 2013

Contributor

jQuery is available if it helps simplify any of the above.

Contributor

mindsocket commented May 1, 2013

I'll move the inline javascript to an included file. As an included panel, is toolbar.js ok for that purpose? If not, where, and should the panel then render the inclusion?

I'll also remove the console logging and replace the load event binding with a jQuery.bind('load'...)

Contributor

mindsocket commented May 1, 2013

I've rebased and updated per suggestions. JS is minified already based on lastest toolbar.js, so hopefully this is good to go.

Owner

jezdez commented May 1, 2013

LGTM!

Contributor

robhudson commented May 1, 2013

Looks great to me too.

This 1000 is here for a reason? I'm guessing its because not all the values are guaranteed to be populated at load. I can't remember off the top of my head which value isn't populated. Ideally there should be loop waiting. I think its loadEventEnd:

https://groups.google.com/d/msg/django-users/hUuBk3lvT-A/gcSX_7o1NvQJ

Owner

mindsocket replied May 2, 2013

I think it's loadEventEnd also, but my vague recollection is that this only applies to one browser. Unfortunately I can't remember which.

"This attribute must return the time when the load event of the current document is completed. It must return zero when the load event is not fired or is not completed."

http://www.w3.org/TR/navigation-timing/#sec-navigation-timing-interface

I would recommend doing a loop, waiting for this to be non-zero, when that occurs, fire the send.

I would also point out that django-statsd does not do this and I am a big fat hypocrite for recommending that you do something the library I wrote does not do.

Owner

mindsocket replied May 16, 2013

My understanding was that a loop would not work since it's running within the load event; that is, unless you are suggesting this be combined with a setTimeout. My javascript-fu isn't strong enough to say for sure, so I'll give it a try.

Owner

mindsocket replied May 16, 2013

For further investigation:
http://lists.w3.org/Archives/Public/public-web-perf/2012Jul/0006.html
"> I think you can use setTimeout with window.onload
Thanks. A 0-length setTimeout worked."

Contributor

robhudson commented May 15, 2013

@mindsocket Is there anything you need to add here based on @andymckay's comment?

Contributor

mindsocket commented May 15, 2013

@robhudson I believe the solution as committed is the only way to ensure loadEventEnd is picked up in all browsers. So, nothing to add AFAIK

Contributor

mindsocket commented May 20, 2013

Committed and pushed a change to the setTimeout call (reduced delay to 0). Tested and working on Firefox and Chrome

Contributor

mindsocket commented May 20, 2013

Ping @andymckay and @robhudson , I think we're close :)

Contributor

mindsocket commented Jun 19, 2013

Is there anything I can do to help get this merged? I'm going to use it in a talk at PyConAU next month and having it in mainline means I can say it's a more readily available tool.

robhudson added a commit that referenced this pull request Jun 21, 2013

Merge pull request #319 from mindsocket/master
Add javascript timing metrics to timing panel onLoad if available

@robhudson robhudson merged commit 7e4ecfd into jazzband:master Jun 21, 2013

ryneeverett pushed a commit to ryneeverett/django-debug-toolbar that referenced this pull request Oct 2, 2016

Merge pull request #319 from mindsocket/master
Add javascript timing metrics to timing panel onLoad if available
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment