329871 - Add profile events to summarize client configuration

Status: RESOLVED FIXED

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

Description

[Brian Ryner (not reading)]

See http://wiki.mozilla.org/Browser_Metrics:Data_Collectors#Profile

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

work in progress

Comment 2

[Brian Ryner (not reading)]

Attachment: patch

A couple of things in this patch that may not be obvious:

- Since the profile collector needs a window (well, a Screen) to generate the profile, we need to wait until the first window is created.  This changes nsMetricsService to observe window (docshell) creation directly, and if the window map is empty, notify the profile collector before notifying the window collector.  This lets us log the profile event as soon as it's possible to.

- If the profile collector is started after the first window is created (this would happen if the initial configuration didn't include the profiel collector, but a later-received one did), it needs to locate a DOM window to use for the Screen object.  I added an accessor to the MetricsService's window map for this, because it seemed like the most convenient way.  Another option would be to use nsIWindowWatcher.

- The MetricsService gets a Hash function which computes a base64'd MD5 hash of a string.  The string is converted to UTF8 first rather than hashing the wide string (makes it easier to check output with "md5sum", and I don't know of any drawback from the hashing point of view...?).

- To log the install date, we want to locate a file which would have been created at install time and not touched by a security update.  defaults/pref/channel-prefs.js works.  The time is rounded off to the nearest day to make it a little less unique.

- Plugin and extension ids are md5 hashed.  This attempts to keep us from inadvertently disclosing unreleased or private extensions.  Since the hash is not reversible, you need to know which extension you're looking for in the output.

Comment 3

[Brian Ryner (not reading)]

Attachment: patch

couple of small cleanups (result code return from CreatePlugin/ExtensionItem is redundant, just check for a null pointer return)

Comment 4

[Brian Ryner (not reading)]

Attachment: patch

I revised the patch to not depend on DOMScreen, and to instead use nsIScreenManager.  I think it's cleaner.  I also added some better error checking for dealing with the plugins/extensions list, and made the failure/null-checking more consistent (prefer a null check to a NS_SUCCEEDED check for getters, since something might fail to report an error).

Note that now the profile collector doesn't need to wait for any window events, and in theory could just be a bunch of static methods without an associated object instance.  I decided against that though, mainly for consistency between the collectors (we'll probably want to formalize an nsIMetricsCollector interface at some point, so that extension-implemented collectors can be enabled based on the config).

Comment 5

[Brian Ryner (not reading)]

Attachment: patch

add number of screens from the screen manager (<display screens="2" ...>)

Comment 6

[Brian Ryner (not reading)]

Attachment: patch

- add <cpu arch="...">
- more logging with MS_LOG

I also updated the wiki for the profile event so that it's consistent with this patch.

Comment 7

[Brian Ryner (not reading)]

Attachment: patch

Updated to build as a standalone extension (see bug 334044)

Comment 8

[Marria Nazif]

+// Logs data on all installed plugins.
+// Should only be used as a stack object in a function.
+class nsProfileCollector::PluginEnumerator

+// Logs data on all installed plugins.
+// Should only be used as a stack object in a function.
+class nsProfileCollector::PluginEnumerator

Any reason that these should only be used as a stack object or is that just a recommendation?

+nsresult
+nsProfileCollector::Init()
+{
+  return LogProfile();

Might want to take this out of Init, since like we talked about we may not want to log profile events only at startup.

+
+nsresult
+nsProfileCollector::AddChildItem(nsIMetricsEventItem *parent,
+                                 const nsAString &childName,
+                                 nsIPropertyBag *childProperties)
+{
+  nsCOMPtr<nsIMetricsEventItem> item;
+  nsMetricsService::get()->CreateEventItem(childName, getter_AddRefs(item));
+  NS_ENSURE_STATE(item);
+
+  item->SetProperties(childProperties);
+
+  nsresult rv = parent->AppendChild(item);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  return NS_OK;
+}

This seems like a good candidate for a function to put in a general place

+  // The file defaults/pref/channel-prefs.js is exlucded from any

exlucded -> excluded

Otherwise looks good to me

Comment 9

[Brian Ryner (not reading)]

(In reply to comment #8)
> Any reason that these should only be used as a stack object or is that just a
> recommendation?

The reason is that they don't hold a reference to the metrics service, so if it went away they would have a dangling pointer.  I could change that (make it a nsRefPtr) and remove that constraint, do you think it's worth it?


> Might want to take this out of Init, since like we talked about we may not want
> to log profile events only at startup.

Yep, I'll make it a separate call from the metrics service (but still at startup for now).

> 
> +
> +nsresult
> +nsProfileCollector::AddChildItem(nsIMetricsEventItem *parent,
> +                                 const nsAString &childName,
> +                                 nsIPropertyBag *childProperties)
> +{
> +  nsCOMPtr<nsIMetricsEventItem> item;
> +  nsMetricsService::get()->CreateEventItem(childName, getter_AddRefs(item));
> +  NS_ENSURE_STATE(item);
> +
> +  item->SetProperties(childProperties);
> +
> +  nsresult rv = parent->AppendChild(item);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return NS_OK;
> +}
> 
> This seems like a good candidate for a function to put in a general place

Good point, I'll move it to nsMetricsUtils.

Comment 10

[Brian Ryner (not reading)]

checked in on branch and trunk

Comment 11

[Brian Ryner (not reading)]

Attachment: fix some missing includes

This adds some includes that were previously brought in through nsIPluginHost (see bug 334870).

Comment 12

[Darin Fisher]

By the way, you could also use NS_Free instead of nsMemory::Free.  NS_Free is defined in nsXPCOM.h and is frozen.

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 219202 [details] [diff] [review]
fix some missing includes

checked in on the trunk and branch, replaced nsMemory::Free with NS_Free

Comment 14

[Brian Ryner (not reading)]

Attachment: addref the metrics service

This addresses Marria's comment about the "stack-based" enumerator objects.  Really what I was trying to indicate is that they assume that a refrence to the metrics service is valid for the entire time they're around.  It's kind of a weird assumption and isn't really necessary - this code doesn't run often enough for refcounting to be a performance bottleneck.

Comment 15

[Marria Nazif]

Comment on attachment 219218 [details] [diff] [review]
addref the metrics service

>Index: extensions/metrics/src/nsProfileCollector.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/metrics/src/nsProfileCollector.cpp,v
>retrieving revision 1.1.2.4
>diff -u -p -r1.1.2.4 nsProfileCollector.cpp
>--- extensions/metrics/src/nsProfileCollector.cpp	20 Apr 2006 22:32:58 -0000	1.1.2.4
>+++ extensions/metrics/src/nsProfileCollector.cpp	20 Apr 2006 23:07:14 -0000
>@@ -50,6 +50,7 @@
> #include "nsIScreenManager.h"
> #include "nsDirectoryServiceUtils.h"
> #include "nsILocalFile.h"
>+#include "nsAutoPtr.h"
> 
> // We need to suppress inclusion of nsString.h
> #define nsString_h___
>@@ -57,7 +58,6 @@
> #undef nsString_h___
> 
> // Logs data on all installed plugins.
>-// Should only be used as a stack object in a function.
> class nsProfileCollector::PluginEnumerator
> {
>  public:
>@@ -73,11 +73,10 @@ class nsProfileCollector::PluginEnumerat
>   // NULL on failure.
>   already_AddRefed<nsIMetricsEventItem> CreatePluginItem(nsIDOMPlugin *plugin);
> 
>-  nsMetricsService *mMetricsService;
>+  nsRefPtr<nsMetricsService> mMetricsService;
> };
> 
> // Logs data on all installed extensions
>-// Should only be used as a stack object in a function.
> class nsProfileCollector::ExtensionEnumerator
> {
>  public:
>@@ -94,7 +93,7 @@ class nsProfileCollector::ExtensionEnume
>   already_AddRefed<nsIMetricsEventItem> CreateExtensionItem(
>       nsIUpdateItem *extension);
> 
>-  nsMetricsService *mMetricsService;
>+  nsRefPtr<nsMetricsService> mMetricsService;
>   nsCOMPtr<nsIRDFService> mRDFService;
>   nsCOMPtr<nsIExtensionManager> mExtensionManager;
>   nsCOMPtr<nsIRDFDataSource> mExtensionsDS;

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 219218 [details] [diff] [review]
addref the metrics service

checked in