1211404 - Limit the length of addon description (& other text fields) in Telemetry

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P2)

Description

[Alessio Placitelli [:Dexter]]

In bug 1187864 comment 3 we found out that some addons are cluttering the pings with unnecessarily long names and descriptions.

We can mitigate this behaviour by setting an arbitrary length limit for the addon name, description & other text fields in Telemetry (100 chars).

Comment 1

[Alessio Placitelli [:Dexter]]

I'm taking this one.

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: bug1211404.patch

Test coverage coming in a separate patch.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8669661 [details] [diff] [review]
bug1211404.patch

Review of attachment 8669661 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +38,5 @@
>  
>  const CHANGE_THROTTLE_INTERVAL_MS = 5 * 60 * 1000;
>  
> +// The maximum length of a string (e.g. addon description) in the environment section.
> +const MAX_ENVIRONMENT_STRING_LENGTH = 100;

We might not want to use the same limit everywhere.
Better make it specifically about the addon data now and revisit later as needed.

`MAX_ADDON_STRING_LENGTH` and update the comment accordingly.

@@ +259,5 @@
> + * @param {Integer} aMaxLength The maximum length of the returned substring. If this is
> + *        greater than the length of the input string, we return the whole input string.
> + * @return {String} The substring or null if the input string is null or undefined.
> + */
> +function getSubstring(aString, aMaxLength) {

getSubstring() is very generic, there is no readability improvement over just directly using .substring() at the callers side.
limitStringToLength() or something like that?

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: bug1211404.patch

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8669736 [details] [diff] [review]
bug1211404.patch

Review of attachment 8669736 [details] [diff] [review]:
-----------------------------------------------------------------

Lets also update the docs as discussed on IRC.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add the length limitation

Thanks. As discussed, I've slightly changed the limitStringToLength to account for the AddonManager not strictly defining the behaviour of the "version" field.

Following up with the docs and the test patches.

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Update the docs.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8669796 [details] [diff] [review]
part 3 - Update the docs.

Review of attachment 8669796 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/docs/environment.rst
@@ +302,5 @@
> +
> +activeAddons
> +~~~~~~~~~~~~
> +
> +Starting from Firefox 44, the length of some string fields (i.e. ``name``, ``description`` and ``version``) is limited to 100 characters. The same limitation applies to the same fields in ``theme`` and ``activePlugins``.

Instead of "some string fields" lets be precise and say "the following string fields: ...".

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8670140 [details] [diff] [review]
part 2 - Add test coverage

Review of attachment 8670140 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below addressed.

::: toolkit/components/telemetry/tests/addons/long-fields/install.rdf
@@ +16,5 @@
> +    </em:targetApplication>
> +
> +    <!-- Front End MetaData -->
> +    <em:name>This is a really long addon name, that will get limited to 100 characters. We're much longer, we're at about 113.</em:name>
> +    <em:description>This is a really long addon description, that will get limited to 100 characters. We're much longer, we're at about 120.</em:description>

I would make them both longer, filling it up with dummy text to 200 chars or so.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +1019,5 @@
>  
> +add_task(function* test_addonsFieldsLimit() {
> +  const ADDON_INSTALL_URL = gDataRoot + "long-fields.xpi";
> +  const ADDON_ID = "[e-mail]";
> +  const ADDON_INSTALL_DATE = truncateToDays(Date.now());

ADDON_INSTALL_DATE is unused, remove it.

@@ +1028,5 @@
> +    "limited to 100 characters. We're much longer, we're at about 116.").substring(0, 100);
> +  const EXPECTED_NAME_STRING = ("This is a really long addon name, that will get limited " +
> +    "to 100 characters. We're much longer, we're at about 113.").substring(0, 100);
> +  const EXPECTED_DESC_STRING = ("This is a really long addon description, that will get " +
> +    "limited to 100 characters. We're much longer, we're at about 120.").substring(0, 100);

I think its enough to just assert that the resulting string lengths are <=100, we don't need to assert & maintain the exact contents.

@@ +1040,5 @@
> +  yield AddonTestUtils.installXPIFromURL(ADDON_INSTALL_URL);
> +
> +  yield deferred.promise;
> +  // Unregister the listener.
> +  TelemetryEnvironment.unregisterChangeListener("test_longFieldsAddon");

Nit: This looks a bit hard to read, maybe:
// Install the addon and wait for the TelemetryEnvironment to pick it up.
let deferred = ...
...registerChangeListener...
yield ...installXPI
yield deferred...
...unregisterChangeListener

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Update the docs.

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage

Comment 13

[Alessio Placitelli [:Dexter]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eccfb4f9dc1b

Comment 14

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/0c5101321917d563783df545ca2d0e72fa8ddc01
Bug 1211404 - Limit the length of addon description (& other text fields) in Telemetry. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/7158aee19845f67ae123bb9b6bb6589c0b3923ad
Bug 1211404 - Add test coverage. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/2e98243f2f7f855d3a5eefb8e208aa2e877d4f02
Bug 1211404 - Update the documentation. r=gfritzsche

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c5101321917
https://hg.mozilla.org/mozilla-central/rev/7158aee19845
https://hg.mozilla.org/mozilla-central/rev/2e98243f2f7f

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: Fx_43 - Folded patches for Beta

Approval Request Comment
[Feature/regressing bug #]: 1211404
[User impact if declined]: This doesn't impact on users. Instead, misbehaving addons reported by Telemetry could be arbitrarily big, increasing server storage costs.
[Describe test coverage new/current, TreeHerder]: This has been on m-c for almost a month now and has xpcshell coverage. There were no reported issues.
[Risks and why]: Low, doesn't change any user facing feature.
[String/UUID change made/needed]: None

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8683069 [details] [diff] [review]
Fx_43 - Folded patches for Beta

Should have no impact to users, has tests & docs, taking it.
Should be in 43 beta 2.

Comment 18

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c0f2fef70f4

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e31f1aa9d84d

Comment 20

[Alessio Placitelli [:Dexter]]

This can be verified by installing this test XPI with some long text fields [0]. Telemetry should limit those fields when reporting the addon.

[0] - https://drive.google.com/a/mozilla.com/file/d/0B1RACVTBwZ94MkQwU0ZYU1ljX1U/view?usp=sharing

Comment 21

[Ada [:adalucinet]]

Reproduced with Nightly 2015-10-08 on Ubuntu 14.04 32-bit.
Verified fixed with 43.0b2 (Build ID: 20151109145326) and 44.0a2 (from 2015-11-10), across platforms [1]: in about:telemetry page - Environment Data/Addons section, with the provided .xpi the value is limited to 100 characters.

[1] Mac OS X 10.11.1, Windows 7 64-bit and Ubuntu 14.04 32-bit