328125 - Add support for client-side throttling

Status: RESOLVED FIXED

(Toolkit Graveyard :: Data Collection/Metrics, enhancement, P1)

Description

[Darin Fisher]

Add support for client-side throttling.

We want to be able to dynamically limit the amount of data that clients send to the server.

Comment 1

[Brian Ryner (not reading)]

Adding some more details based on some further refinement of the design.  We'd like to be able to have the collection server specify the following data in a response (either a response to an upload, or a "ping" response):

- which data collectors should be enabled
- maximum number of events to record per logging interval
- time duration until the next scheduled upload

Comment 2

[Darin Fisher]

-> me

Comment 3

[Darin Fisher]

Sample XML file describing test parameters:

<config xmlns="http://www.mozilla.org/metrics">
  <collectors>
    <collector type="ui"/>
    <collector type="load"/>
    <collector type="window"/>
  </collectors>
  <limit events="200"/>
  <upload interval="600"/>
</config>

Comment 4

[Darin Fisher]

Attachment: v1 patch

Initial cut.  What do you think?

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 215312 [details] [diff] [review]
v1 patch

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ nsMetricsConfig.cpp	16 Mar 2006 20:37:52 -0000
>+#define DEFAULT_UPLOAD_INTERVAL 3600  // 1 hour

Hm, so this will ping the server once an hour and get a new configuration.  I'm not sure that's quite what we want -- it seems like this case should be interpreted as "never", and we'll ping again at the start of the next session.

>+nsresult
>+nsMetricsConfig::Load(nsIFile *file)
>+{
>+  NS_ENSURE_STATE(mEventSet.IsInitialized());

It seems like we should do this in an Init() method so that we don't have to check this once the object is set up.

>+  // At this point, we can clear our old configuration.
>+  mEventSet.Clear();
>+  mEventLimit = 0;
>+  mUploadInterval = DEFAULT_UPLOAD_INTERVAL;

Could just be a call to Reset(), right?

>+void
>+nsMetricsConfig::ForEachChildElement(nsIDOMElement *elem,
>+                                     ForEachChildElementCallback callback)
>+{
>+  nsCOMPtr<nsIDOMNode> node, next;
>+  elem->GetFirstChild(getter_AddRefs(node));
>+  while (node) {
>+    PRUint16 nodeType;
>+    node->GetNodeType(&nodeType);
>+    if (nodeType == nsIDOMNode::ELEMENT_NODE) {
>+      nsCOMPtr<nsIDOMElement> childElem = do_QueryInterface(node);
>+      if (childElem)
>+        (this->*callback)(childElem);

It would be slightly more performant to just unconditionally QI to nsIDOMElement.  It's always true that nodeType == ELEMENT_NODE if the node implements nsIDOMElement.

>+void
>+nsMetricsConfig::ProcessToplevelElement(nsIDOMElement *elem)
>+{
>+  // Process a top-level element
>+  nsAutoString name;
>+  elem->GetLocalName(name);
>+  if (name.EqualsLiteral("collectors")) {

We should probably be enforcing the namespace here (and elsewhere) by checking namespaceURI.  It's a little inefficient with the DOM API, but we shouldn't be running this very frequently (nsIContent::GetNameSpaceID would be an option, if it's a problem).

>+    // Enumerate <collector> elements
>+    ForEachChildElement(elem, &nsMetricsConfig::ProcessCollectorElement);
>+  } else if (name.EqualsLiteral("limit")) {
>+    ReadIntegerAttr(elem, NS_LITERAL_STRING("events"), &mEventLimit);

You should comment somewhere (here, or at ReadIntegerAttr) that it leaves the output unmodified if the attribute doesn't parse as an integer.

>+  } else if (name.EqualsLiteral("upload")) {
>+    ReadIntegerAttr(elem, NS_LITERAL_STRING("interval"), &mUploadInterval);
>+  }

Here and many other places -- it would be cleaner to define these strings as constants somewhere?

>--- nsMetricsService.cpp	7 Mar 2006 05:31:32 -0000	1.5
>+++ nsMetricsService.cpp	16 Mar 2006 20:37:52 -0000
>@@ -85,20 +86,28 @@ NS_IMPL_ISUPPORTS6_CI(nsMetricsService, 
> NS_IMETHODIMP
> nsMetricsService::LogEvent(const nsAString &eventNS,
>                            const nsAString &eventName,
>                            nsIPropertyBag *eventProperties)
> {
>   NS_ENSURE_ARG_POINTER(eventProperties);
> 
>   if (mSuspendCount != 0)  // Ignore events while suspended
>     return NS_OK;
> 
>+  // Restrict the number of events logged
>+  if (mEventCount >= mConfig.EventLimit())
>+    return NS_OK;
>+
>+  // Restrict the types of events logged
>+  if (!mConfig.IsEventEnabled(eventNS, eventName))
>+    return NS_OK;
>+

Hm, I thought we decided to make this actually start up / shut down the collectors.  We may need this check as well though, because of dependencies the collectors have on each other (all of our current collectors depend on the window collector, for example).

>@@ -158,21 +167,21 @@ nsMetricsService::LogEvent(const nsAStri
>   // Add the event timestamp
>   nsAutoString timeString;
>   timeString.AppendInt(PR_Now() / PR_USEC_PER_SEC);
>   rv = eventElement->SetAttribute(NS_LITERAL_STRING("time"), timeString);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   nsCOMPtr<nsIDOMNode> outChild;
>   rv = mRoot->AppendChild(eventElement, getter_AddRefs(outChild));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  if (++mEventCount > NS_EVENTLOG_FLUSH_POINT)
>+  if ((++mEventCount % NS_EVENTLOG_FLUSH_POINT) == 0)

Can you explain this change?

> NS_IMETHODIMP
> nsMetricsService::OnStopRequest(nsIRequest *request, nsISupports *context,
>                                 nsresult status)
> {
>+  if (mConfigOutputStream) {
>+    mConfigOutputStream->Close();
>+    mConfigOutputStream = 0;
>+  }
>+
>+  // Load configuration file:
>+
>+  nsCOMPtr<nsIFile> file;
>+  GetConfigFile(getter_AddRefs(file));
>+
>+  if (NS_SUCCEEDED(status))

This will fail if we couldn't create the file, full disk, etc, right?

> NS_IMETHODIMP
> nsMetricsService::Observe(nsISupports *subject, const char *topic,
>                           const PRUnichar *data)
> {
>-      mgr->RegisterTimer(NS_LITERAL_STRING("metrics-upload"), this, interval);
>+      mgr->RegisterTimer(NS_LITERAL_STRING("metrics-upload"), this,
>+                         mConfig.UploadInterval() * 1000);

maybe use PR_MSEC_PER_SEC, for clarity?

>@@ -359,20 +397,30 @@ nsMetricsService::Create(nsISupports *ou
>+  // Initialize configuration.
>+  nsCOMPtr<nsIFile> file;
>+  GetConfigFile(getter_AddRefs(file));
>+  if (file)
>+    mConfig.Load(file);
>+
>+  nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+  if (prefs)

Seems like we should fail to init instead of silently continuing if prefs is null.

r=me with those fixed

Comment 6

[Darin Fisher]

(In reply to comment #5)
> (From update of attachment 215312 [details] [diff] [review] [edit])
> >--- /dev/null	1 Jan 1970 00:00:00 -0000
> >+++ nsMetricsConfig.cpp	16 Mar 2006 20:37:52 -0000
> >+#define DEFAULT_UPLOAD_INTERVAL 3600  // 1 hour
> 
> Hm, so this will ping the server once an hour and get a new configuration.  

Not quite.  The first time the user runs the browser with the metrics service, there will be a one hour delay before the browser pings the server to get a configuration.  From that point forward, it will use the configuration supplied by the server.  If the configuration from the server is malformed, or if the upload request fails, then the browser will revert to waiting one hour before sending a ping to the server again.


> I'm
> not sure that's quite what we want -- it seems like this case should be
> interpreted as "never", and we'll ping again at the start of the next 
> session.

I could do that.  Is there a reason why that is better?



> >+nsresult
> >+nsMetricsConfig::Load(nsIFile *file)
> >+{
> >+  NS_ENSURE_STATE(mEventSet.IsInitialized());
> 
> It seems like we should do this in an Init() method so that we don't have to
> check this once the object is set up.

Sure, I'll add an Init method that is called from nsMetricsService::Init.


> >+  // At this point, we can clear our old configuration.
> >+  mEventSet.Clear();
> >+  mEventLimit = 0;
> >+  mUploadInterval = DEFAULT_UPLOAD_INTERVAL;
> 
> Could just be a call to Reset(), right?

Yup, good catch.  I added Reset to nsMetricsConfig after I had written that code.


> It would be slightly more performant to just unconditionally QI to
> nsIDOMElement.  It's always true that nodeType == ELEMENT_NODE if the node
> implements nsIDOMElement.

Excellent.  I'll make that change.


> We should probably be enforcing the namespace here (and elsewhere) ...

I thought about doing that, but since this is our configuration file, I didn't think it would be necessary.  We fail gracefully if an expected attribute is missing, etc. or if an attribute value does not parse properly.  Is there really much advantage to verifying the namespace here?


> >+    // Enumerate <collector> elements
> >+    ForEachChildElement(elem, &nsMetricsConfig::ProcessCollectorElement);
> >+  } else if (name.EqualsLiteral("limit")) {
> >+    ReadIntegerAttr(elem, NS_LITERAL_STRING("events"), &mEventLimit);
> 
> You should comment somewhere (here, or at ReadIntegerAttr) that it leaves the
> output unmodified if the attribute doesn't parse as an integer.

OK


> >+  } else if (name.EqualsLiteral("upload")) {
> >+    ReadIntegerAttr(elem, NS_LITERAL_STRING("interval"), &mUploadInterval);
> >+  }
> 
> Here and many other places -- it would be cleaner to define these strings as
> constants somewhere?

Hmm, ok.


> Hm, I thought we decided to make this actually start up / shut down the
> collectors.  We may need this check as well though, because of dependencies the
> collectors have on each other (all of our current collectors depend on the
> window collector, for example).

Yeah, we did discuss that, and I agree that it is a good idea.  I think we should do this at least.


> >-  if (++mEventCount > NS_EVENTLOG_FLUSH_POINT)
> >+  if ((++mEventCount % NS_EVENTLOG_FLUSH_POINT) == 0)
> 
> Can you explain this change?

We had a bug.  We want to flush at each multiple of NS_EVENTLOG_FLUSH_POINT.
mEventCount measures the total number of events logged, including those stored on disk but not in memory.


> >+  if (NS_SUCCEEDED(status))
> 
> This will fail if we couldn't create the file, full disk, etc, right?

Yes.  Do you think that is problematic?


> >+      mgr->RegisterTimer(NS_LITERAL_STRING("metrics-upload"), this,
> >+                         mConfig.UploadInterval() * 1000);
> 
> maybe use PR_MSEC_PER_SEC, for clarity?

Sure


> >+  nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
> >+  if (prefs)
> 
> Seems like we should fail to init instead of silently continuing if prefs is
> null.

Yeah, OK

Comment 7

[Darin Fisher]

fixed-on-trunk and fixed1.8.1