1477750 - Documentation on limits of event fields is incorrect

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Jan-Erik Rediger [:janerik]]

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

We'll check with Chris next week if he can look into this.

Comment 2

[Chris H-C :chutten]

Could you provide the missing reference for [1]?

Comment 3

[Jan-Erik Rediger [:janerik]]

[1]: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/events.html#limits

Comment 4

[Chris H-C :chutten]

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

Comment 5

[Chris H-C :chutten]

Attachment: bug 1477750 - Correct the 'event' identifier pattern documentation r?janerik

Comment 6

[Phabricator Automation]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0e9a6ae0f65
Correct the 'event' identifier pattern documentation r=janerik

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/d0e9a6ae0f65