Closed Bug 329871 Opened 18 years ago Closed 18 years ago

Add profile events to summarize client configuration

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bryner, Assigned: bryner)

References

Details

Attachments

(3 files, 6 obsolete files)

Depends on: 332172
Depends on: 332640
Assignee: nobody → bryner
Attached patch patch (obsolete) — Splinter Review
work in progress
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #217250 - Attachment is obsolete: true
Attachment #217519 - Flags: first-review?(marria)
Attached patch patch (obsolete) — Splinter Review
couple of small cleanups (result code return from CreatePlugin/ExtensionItem is redundant, just check for a null pointer return)
Attachment #217519 - Attachment is obsolete: true
Attachment #217574 - Flags: first-review?(marria)
Attachment #217519 - Flags: first-review?(marria)
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #217574 - Attachment is obsolete: true
Attachment #217718 - Flags: first-review?(marria)
Attachment #217574 - Flags: first-review?(marria)
Attached patch patch (obsolete) — Splinter Review
add number of screens from the screen manager (<display screens="2" ...>)
Attachment #217718 - Attachment is obsolete: true
Attachment #217902 - Flags: first-review?(marria)
Attachment #217718 - Flags: first-review?(marria)
Attached patch patch (obsolete) — Splinter Review
- 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.
Attachment #217902 - Attachment is obsolete: true
Attachment #218139 - Flags: first-review?(marria)
Attachment #217902 - Flags: first-review?(marria)
No longer depends on: 332640
Attached patch patchSplinter Review
Updated to build as a standalone extension (see bug 334044)
Attachment #218139 - Attachment is obsolete: true
Attachment #218911 - Flags: first-review?(marria)
Attachment #218139 - Flags: first-review?(marria)
+// 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

Attachment #218911 - Flags: first-review?(marria) → first-review+
(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.
checked in on branch and trunk
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
This adds some includes that were previously brought in through nsIPluginHost (see bug 334870).
Attachment #219202 - Flags: first-review?(darin)
Attachment #219202 - Flags: first-review?(darin) → first-review+
By the way, you could also use NS_Free instead of nsMemory::Free.  NS_Free is defined in nsXPCOM.h and is frozen.
Comment on attachment 219202 [details] [diff] [review]
fix some missing includes

checked in on the trunk and branch, replaced nsMemory::Free with NS_Free
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.
Attachment #219218 - Flags: first-review?(marria)
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;
Attachment #219218 - Flags: first-review?(marria) → first-review+
Comment on attachment 219218 [details] [diff] [review]
addref the metrics service

checked in
No longer blocks: 328064
Flags: first-review+
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: