Closed Bug 1477750 Opened 3 years ago Closed 3 years ago

Documentation on limits of event fields is incorrect

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: janerik, Assigned: chutten)

References

Details

Attachments

(1 file)

The documentation on [1] lists a single regex for all identifiers (category, method, object, key), but :scolville mentioned it is not fully accurate.

We should: 

* Check the current limits
* Precisely document the limits per field
* Document why these limits are chosen
We'll check with Chris next week if he can look into this.
Could you provide the missing reference for [1]?
Flags: needinfo?(jrediger)
The code checking whether identifier strings are valid actually does this:

1) Check the length
2) Ensure all characters are ascii alphanum [a-zA-Z0-9]
3) Optionally allow '.' or '_' infix (that is, not as first or last)

category strings are 30, with infix '.' and '_' allowed
name strings are 20 with infix '_'
method strings are 20 with infix '_'
object strings are 20 with infix '_'
extra key strings are 15 with infix '_'

These are all checked at registration. Failures at registration abort registration so a single invalid registration will fail the whole call. Failures are logged and appropriate NS_ERROR_* retvals are returned (which I think turn into exceptions? Not sure if we eat them somewhere along the line). We do not truncate identifiers to make them fit.

At recording, meanwhile, we check:
value strings are 80 bytes, no character restriction
extra must be an object with string keys and string values, and extra values are 80 bytes, no character restriction
(interestingly we don't validate provided extra keys at recording time, we instead compare them to the registered set after they reach the main process. There may be a bug in this)

In the event that `value` or `extra`'s values are larger than their 80 bytes we truncate (depending on UTF8 multibyte sequence boundaries we may truncate behind 80 bytes). We log on errors, including on the non-fatal case when we truncate.

So it does appear as though we need to update our documentation, especially about how infix '.' is only allowed in `category`.

Relevant code:
RegisterEvents https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/toolkit/components/telemetry/TelemetryEvent.cpp#986
RecordEvent https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/toolkit/components/telemetry/TelemetryEvent.cpp#450
IsValidIdentifierString https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/toolkit/components/telemetry/TelemetryCommon.cpp#169
See Also: → 1476349
Assignee: nobody → chutten
Blocks: 1476349
Status: NEW → ASSIGNED
Priority: -- → P1
See Also: 1476349
Comment on attachment 8997523 [details]
bug 1477750 - Correct the 'event' identifier pattern documentation r?janerik

Jan-Erik Rediger [:janerik] has approved the revision.

https://phabricator.services.mozilla.com/D2735
Attachment #8997523 - Flags: review+
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d0e9a6ae0f65
Correct the 'event' identifier pattern documentation r=janerik
https://hg.mozilla.org/mozilla-central/rev/d0e9a6ae0f65
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.