330712 - generate, store, and send a user id for data collection

Status: RESOLVED FIXED

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

Description

[Brian Ryner (not reading)]

We should generate a sufficiently random 64-bit id the first time we need to upload metrics data, and store it (in a pref would be easiest).  This should be sent as an attribute of the root <log> element.

Comment 1

[Brian Ryner (not reading)]

I think we'd actually like to use a guid string for this.  The easiest way is probably to land the UUID generator on the branch.

Comment 2

[Brian Ryner (not reading)]

Attachment: patch

Comment 3

[Darin Fisher]

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

>Index: nsMetricsService.cpp

>+  // footer.  We need to generate a client id now if one doesn't exist.
>+  static const char kClientIDPref[] = "metrics.client_id";

We also have "metrics.event-count", so either change that to "_" or change
this one to use "-"


>+  char *head = PR_smprintf(METRICS_XML_HEAD, clientID.get());
>+  
>   nsCOMPtr<nsIInputStream> stringStream;
>+  NS_NewByteInputStream(getter_AddRefs(stringStream), head, -1,
>+                        NS_ASSIGNMENT_COPY);
>+  PR_smprintf_free(head);

Hrm.. we should have a version of PR_smprintf that returns a buffer that
can be freed using NS_Free.  Then you could just use NS_ASSIGNMENT_ADOPT.


>+  char *idstr = id.ToString();
>+  NS_ENSURE_STATE(idstr);
>+
>+  // Strip off the enclosing curly brackets
>+  clientID.Assign(idstr + 1, 36);
>+  PR_Free(idstr);

Hrm... it'd also be nice if you just use clientID.Adopt here.  We
should just document the fact that NS_Free == PR_Free, and then
that would be possible.  A version of ToString that wrote into a
nsCString would also be nice.

Comment 4

[Brian Ryner (not reading)]

(In reply to comment #3)
> We also have "metrics.event-count", so either change that to "_" or change
> this one to use "-"

Changed the new one.

> Hrm... it'd also be nice if you just use clientID.Adopt here.  We
> should just document the fact that NS_Free == PR_Free, and then
> that would be possible.  A version of ToString that wrote into a
> nsCString would also be nice.
> 

Which of these, if any, do you think I need to do before checking in this patch?

Comment 5

[Brian Ryner (not reading)]

Attachment: use a header instead

This is actually somewhat preferable because the server can inspect the ID prior to parsing the XML.

Comment 6

[Darin Fisher]

Comment on attachment 215745 [details] [diff] [review]
use a header instead

>Index: nsMetricsService.cpp

>+  rv = httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("X-Moz-ClientID"),

Somehow, X-Moz-ClientID feels more "evil" than the XML clientid attribute.
I'm not sure why ;-)

You probably want to put a hyphen between Client and ID, so:

  X-Moz-Client-ID

Also, it might make sense to reuse the already existing 'X-Moz' header:

  X-Moz: mcid=N

where "mcid" stands for Metrics Client ID.  Or, is that just too ugly? :-P

The 'X-Moz' header is currently sent with prefetch requests:
 
  X-Moz: prefetch


>+  // Strip off the enclosing curly brackets
>+  clientID.Assign(idstr + 1, 36);

Add an assertion that strlen(idstr) == 38 ?  Maybe add a constant for 36 ?

Comment 7

[Brian Ryner (not reading)]

(In reply to comment #6)
> You probably want to put a hyphen between Client and ID, so:
> 
>   X-Moz-Client-ID
> 

Sure.

> Also, it might make sense to reuse the already existing 'X-Moz' header:
> 
>   X-Moz: mcid=N
> 
> where "mcid" stands for Metrics Client ID.  Or, is that just too ugly? :-P

Hm, that's somewhat more work to parse, and I don't think it buys us much.  I'll stick with X-Moz-Client-ID.

> >+  // Strip off the enclosing curly brackets
> >+  clientID.Assign(idstr + 1, 36);
> 
> Add an assertion that strlen(idstr) == 38 ?  Maybe add a constant for 36 ?
> 

ok.

Comment 8

[Darin Fisher]

> Hm, that's somewhat more work to parse, and I don't think it buys us much. 
> I'll stick with X-Moz-Client-ID.

OK

Comment 9

[Brian Ryner (not reading)]

checked in (branch and trunk)

Comment 10

[Brian Ryner (not reading)]

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

After some more discussion, we decided to go back to the attribute approach for consistency.  I'm reverting to that patch and committing it, since it was already reviewed.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 215745 [details] [diff] [review]
use a header instead

not using this one.