331166 - attach a session id to all events

Status: RESOLVED FIXED

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

Description

[Brian Ryner (not reading)]

We need to keep a session id that increments each time the browser is started, to provide a context for DOM window ids (which count from 0 in each new session).

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

This adds the session id, and fixes several other problems I found while testing:

- We need to flush the log when we get quit-application, not xpcom-shutdown.  The XML serializer crashes when invoked during xpcom-shutdown.
- At startup we should only enable the collectors that are given in the config file, not all collectors.  If there is no config file we should immediately upload so that we can get one.
- Profile prefs can't be read or set in Init(), we have to wait for the profile-after-change notification.  I moved the bulk of initialization to this function.  There's really no reason why anything should need to happen in Init().
- The collectors need to be tolerant of being shut down if already disabled
- A little cleanup in nsMetricsConfig, and the default event limit should be "no limit".
- Only try to load a config file at startup if one exists.  This is mainly to prevent a benign warning.

Comment 2

[Marria Nazif]

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

> 
> void
> nsMetricsConfig::Reset()
> {
>   mEventSet.Clear();
>-  mEventLimit = 0;
>+  mEventLimit = PR_INT32_MAX; // no limit by default
>   mUploadInterval = NS_DEFAULT_UPLOAD_INTERVAL;
> }

Doesn't this actually make more sense being 0, since we don't want the client sending any events until it recieves a reply config from the server?

>+  // Start up the collectors
>+  rv = nsWindowCollector::Startup();
>+  NS_ENSURE_SUCCESS(rv, rv);

Does this mean we will always collect window events even if the server asks not to collect anything?

>+  // If we didn't load a config, immediately upload our empty log.
>+  // This will allow us to receive a config file from the server.
>+  if (!loaded) {
>+    Upload();
>   }

What happens if this fails - I assume it just tries again at the usual interval?  I wonder if we want to have a longer interval for inactive clients to prevent unneccessary server pings?

Otherwise looks good

Comment 3

[Brian Ryner (not reading)]

(In reply to comment #2)
> (From update of attachment 215951 [details] [diff] [review] [edit])
> > 
> > void
> > nsMetricsConfig::Reset()
> > {
> >   mEventSet.Clear();
> >-  mEventLimit = 0;
> >+  mEventLimit = PR_INT32_MAX; // no limit by default
> >   mUploadInterval = NS_DEFAULT_UPLOAD_INTERVAL;
> > }
> 
> Doesn't this actually make more sense being 0, since we don't want the client
> sending any events until it recieves a reply config from the server?

Well, my intent was that the server response wouldn't have to contain a <limit> line if it doesn't want to cap the number of events.  In any case, the client won't collect any events until it's told to enable at least one collector.

> >+  // Start up the collectors
> >+  rv = nsWindowCollector::Startup();
> >+  NS_ENSURE_SUCCESS(rv, rv);
> 
> Does this mean we will always collect window events even if the server asks not
> to collect anything?

Sorta.  There's a check in LogEvent that will prevent them from actually being logged, but the window collector doubles as the window ID map, and if it's not initialized, the window ID lookup doesn't work.  I think we should separate the two functions, since they're fairly independent - I can do that in this patch or in a separate one.

> >+  // If we didn't load a config, immediately upload our empty log.
> >+  // This will allow us to receive a config file from the server.
> >+  if (!loaded) {
> >+    Upload();
> >   }
> 
> What happens if this fails - I assume it just tries again at the usual
> interval?  I wonder if we want to have a longer interval for inactive clients
> to prevent unneccessary server pings?

It'll try again after 1 hour, and fall off if it fails again, down to once per day if it continues to fail.  See nsMetricsService::OnStopRequest().  I'll add a comment here to try to clarify what happens if the initial ping fails.

Comment 4

[Marria Nazif]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 215951 [details] [diff] [review] [edit] [edit])
> > > 
> > > void
> > > nsMetricsConfig::Reset()
> > > {
> > >   mEventSet.Clear();
> > >-  mEventLimit = 0;
> > >+  mEventLimit = PR_INT32_MAX; // no limit by default
> > >   mUploadInterval = NS_DEFAULT_UPLOAD_INTERVAL;
> > > }
> > 
> > Doesn't this actually make more sense being 0, since we don't want the client
> > sending any events until it recieves a reply config from the server?
> 
> Well, my intent was that the server response wouldn't have to contain a <limit>
> line if it doesn't want to cap the number of events.  In any case, the client
> won't collect any events until it's told to enable at least one collector.

ah, ok, that makes sense.

> > >+  // Start up the collectors
> > >+  rv = nsWindowCollector::Startup();
> > >+  NS_ENSURE_SUCCESS(rv, rv);
> > 
> > Does this mean we will always collect window events even if the server asks not
> > to collect anything?
> 
> Sorta.  There's a check in LogEvent that will prevent them from actually being
> logged, but the window collector doubles as the window ID map, and if it's not
> initialized, the window ID lookup doesn't work.  I think we should separate the
> two functions, since they're fairly independent - I can do that in this patch
> or in a separate one.

ok, you can do that in a separate patch

> > >+  // If we didn't load a config, immediately upload our empty log.
> > >+  // This will allow us to receive a config file from the server.
> > >+  if (!loaded) {
> > >+    Upload();
> > >   }
> > 
> > What happens if this fails - I assume it just tries again at the usual
> > interval?  I wonder if we want to have a longer interval for inactive clients
> > to prevent unneccessary server pings?
> 
> It'll try again after 1 hour, and fall off if it fails again, down to once per
> day if it continues to fail.  See nsMetricsService::OnStopRequest().  I'll add
> a comment here to try to clarify what happens if the initial ping fails.
> 

cool, thanks for the pointer.

looks good

Comment 5

[Brian Ryner (not reading)]

checked in (trunk and branch)

Comment 6

[Brian Ryner (not reading)]

Attachment: fix assertion

The first patch here introduced a bug in nsMetricsConfig intialization - mEventSet.Clear() is now called before mEventSet.Init().  This patch fixes that, adds some assertions to catch this in a debug build, and also adds a couple of easy missing "const"s.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 216476 [details] [diff] [review]
fix assertion

fix checked in