Closed Bug 330712 Opened 17 years ago Closed 17 years ago

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

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bryner, Assigned: bryner)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Depends on: 279521
Attached patch patchSplinter Review
Assignee: nobody → bryner
Status: NEW → ASSIGNED
Attachment #215724 - Flags: first-review?(darin)
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.
Attachment #215724 - Flags: first-review?(darin) → first-review+
(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?
Attached patch use a header instead (obsolete) — Splinter Review
This is actually somewhat preferable because the server can inspect the ID prior to parsing the XML.
Attachment #215724 - Attachment is obsolete: true
Attachment #215745 - Flags: first-review?(darin)
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 ?
Attachment #215745 - Flags: first-review?(darin) → first-review+
(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.
> 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
checked in (branch and trunk)
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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.
Attachment #215724 - Attachment is obsolete: false
Comment on attachment 215745 [details] [diff] [review]
use a header instead

not using this one.
Attachment #215745 - Attachment is obsolete: true
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.