Closed Bug 326707 Opened 15 years ago Closed 15 years ago

Collect data about load events

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bryner, Assigned: bryner)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 326742
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #211462 - Flags: second-review?(darin)
Attachment #211462 - Flags: first-review?(marria)
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.
Depends on: 326784
Attached patch patch (obsolete) — Splinter Review
This should have the above problems fixed.
Attachment #211462 - Attachment is obsolete: true
Attachment #211545 - Flags: second-review?(darin)
Attachment #211545 - Flags: first-review?(marria)
Attachment #211462 - Flags: second-review?(darin)
Attachment #211462 - Flags: first-review?(marria)
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #211545 - Attachment is obsolete: true
Attachment #211551 - Flags: second-review?(darin)
Attachment #211551 - Flags: first-review?(marria)
Attachment #211545 - Flags: second-review?(darin)
Attachment #211545 - Flags: first-review?(marria)
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
Attachment #211551 - Flags: second-review?(darin) → second-review+
Attached patch patchSplinter Review
Attachment #211551 - Attachment is obsolete: true
Attachment #212177 - Flags: second-review?(darin)
Attachment #212177 - Flags: first-review?(marria)
Attachment #211551 - Flags: first-review?(marria)
Comment on attachment 212177 [details] [diff] [review]
patch

looks good.  so little remains of the original version(s)!
Attachment #212177 - Flags: second-review?(darin) → second-review+
(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?
checked in; will address any other review comments separately.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
QA Contact: nobody → metrics
Attachment #212177 - Flags: first-review?(marria)
Flags: second-review+
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.