326707 - Collect data about load events

Status: RESOLVED FIXED

(Toolkit Graveyard :: Data Collection/Metrics, defect)

Description

[Brian Ryner (not reading)]

This will be further specified on http://wiki.mozilla.org/Browser_Metrics:Data_Collectors , but basically this includes the type of load (how it was initiated), the target window, elapsed time, and some data about memory consumption.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

This implements logging for load events and window create/open/close/destroy.  Each window (both toplevel and subframe) is given a unique id which is included in all load events for that window.  This code doesn't quite handle redirects correctly -- no STATE_START is received for the start of the redirected load.  I'll continue to investigate that; I don't think it necessarily needs to work for this to land though.

Comment 2

[Brian Ryner (not reading)]

I found a couple more problems with this:

- opener doesn't log correctly because it's not yet set when the notification happens.  I'm working on sorting this out (I had tested with a custom notification I'd added in nsGlobalWindow::OpenInternal, then switched to the existing 'domwindowopened' notification, not realizing that the timing is different).

- Integer values need to have their byte order normalized.

Expect a new patch with those problems addressed, but any other feedback on this version would be great.

Comment 3

[Brian Ryner (not reading)]

Attachment: patch

This should have the above problems fixed.

Comment 4

[Brian Ryner (not reading)]

Attachment: patch

Fixes a couple of other problems I noticed when I tried to get this to actually upload:

- The registered category id needs to be prefixed by "service,", or createInstance will be used, possibly creating a second instance of the metrics service.
- When the app-startup notification happens, the pref service hasn't yet initialized the profile prefs, so the update timer doesn't get set.  The fix is to do this part using the profile-after-change notification.

I also fixed an incorrect comment in nsMetricsEvent.h (event type is 8 bits, not 16 bits).

Comment 5

[Darin Fisher]

Comment on attachment 211551 [details] [diff] [review]
patch

>Index: src/nsLoadCollector.cpp

>+  nsCOMPtr<nsIChannel> channel = do_QueryInterface(request);
>+  if (!channel) {
>+    // We don't care about non-channel requests
>+    return NS_OK;
>+  }

This includes a lot of requests.  We may even wish to limit our
logging to HTTP requests.  Or, perhaps we should log the URL scheme
so we can easily filter on the protocol type (to at least allow us
to exclude chrome URLs).


>+  gLoadCollector = new nsLoadCollector();
...
>+nsLoadCollector::Shutdown()
>+{
>+  // See comments in nsWindowCollector::Shutdown about why we don't
>+  // null out gLoadCollector here.
>+  gLoadCollector->Release();
>+}

Can gLoadCollector be allocated statically instead of on the heap?


>Index: src/nsMetricsModule.cpp

>   cat->AddCategoryEntry("app-startup",
>                         NS_METRICSSERVICE_CLASSNAME,
>-                        NS_METRICSSERVICE_CONTRACTID,
>+                        "service," NS_METRICSSERVICE_CONTRACTID,
>                         PR_TRUE, PR_TRUE, nsnull);

my apologies for the startup bugs


>   nsMetricsService()
>-    : mSuspendCount(0)
>-    , mUploading(PR_FALSE)
>-  {}
>-
>+      : mSuspendCount(0),
>+        mUploading(PR_FALSE)

/me frowns at coding style change! ;-)


>+  static nsMetricsService* GetMetricsService()
>+  {
>+    if (!sMetricsService) {
>+      nsCOMPtr<nsIMetricsService> ms =
>+        do_GetService(NS_METRICSSERVICE_CONTRACTID);
>+    }
>+    return sMetricsService;
>+  }

We have to be careful about using GetMetricsService after
XPCOM shutdown.  In that case, this function may return null.


r=darin

Comment 6

[Brian Ryner (not reading)]

Attachment: patch

Comment 7

[Darin Fisher]

Comment on attachment 212177 [details] [diff] [review]
patch

looks good.  so little remains of the original version(s)!

Comment 8

[Brian Ryner (not reading)]

(In reply to comment #5)
> This includes a lot of requests.  We may even wish to limit our
> logging to HTTP requests.  Or, perhaps we should log the URL scheme
> so we can easily filter on the protocol type (to at least allow us
> to exclude chrome URLs).

Let's examine that once we have a better idea what typical data streams will look like.

> Can gLoadCollector be allocated statically instead of on the heap?

Hm, it would involve overriding AddRef and Release, and would be somewhat nasty with the weak reference stuff.  I don't think it's really worth it, unless I'm missing something...

> We have to be careful about using GetMetricsService after
> XPCOM shutdown.  In that case, this function may return null.

What can we do to avoid this problem?

Comment 9

[Brian Ryner (not reading)]

checked in; will address any other review comments separately.

Comment 10

[Brian Ryner (not reading)]

Oh, just for reference, I posted a description of the XML format used here:

http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/e8f2513c2af195ea/cd31b08b0f552c7e#cd31b08b0f552c7e