1162476 - Telemetry should reject external pings with invalid types

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Currently TelemetryController.submitExternalPing() blindly to use a type string of the format we expect.
For simplicity we should reject everything which is not of the format: /^[a-z0-9][a-z0-9-]+[a-z0-9]$/i

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Telemetry should reject external pings with invalid types

Simple patch, i think that's pretty obvious. If you start submitting a new ping type you should test that anyway, so this rejecting on invalid types should not break anything released.

Comment 2

[Mark Reid [:mreid]]

Should we do this rejection on the server-side to potentially discover bugs or improperly-added ping types?

Comment 3

[Georg Fritzsche [:gfritzsche]]

Ok, there is two problems there:
* discovering bugs
* keeping the Telemetry client code working properly

For discovering bugs we could use a keyed histogram counter ("count of rejected pings", key being the type).
We use the type internally for things like archiving filenames, so we shouldn't leave that through.

Comment 4

[Georg Fritzsche [:gfritzsche]]

An alternative approach:
If an external ping with an invalid ping type is submitted, set the ping type to "invalid" and store the invalid type in a different field (say ping.submittedType).

Comment 5

[Mark Reid [:mreid]]

I think a keyed histogram counter (with proper monitoring / alerting on the server) would cover the case I mentioned in Comment 2.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Attachment: Telemetry should reject external pings with invalid types

This adds the probe for counting invalid ping types. I also moved the two lonely TELEMETRY_* histograms up to the rest and added alerts.

Comment 7

[Benjamin Smedberg]

I think that measuring invalid ping types is over-engineering this. They won't work, and that will be caught in QA. And we don't have the staffing to really monitor this anyway.

Comment 8

[Vladan Djeric (:vladan)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> I think that measuring invalid ping types is over-engineering this. They
> won't work, and that will be caught in QA. And we don't have the staffing to
> really monitor this anyway.

Just as an aside, changes in histogram distributions are monitored automatically by Roberto's change detection tool

Comment 9

[Vladan Djeric (:vladan)]

Comment on attachment 8602860 [details] [diff] [review]
Telemetry should reject external pings with invalid types

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

::: toolkit/components/telemetry/Histograms.json
@@ +4388,5 @@
>      "n_values": 30,
>      "description": "Number of telemetry pings evicted at startup"
>    },
> +  "TELEMETRY_PING": {
> +    "alert_emails": ["[e-mail]"],

You don't need to set [e-mail], all histograms generate alerts even without an alert_emails field, and the alerts always get sent to the default notification address. The alert_emails field is intended for identifying teams which own the probe

Btw, I might have mislead you last week, the default notification address is actually [e-mail], the alerts get sent FROM [e-mail]

@@ +4403,5 @@
> +    "kind": "boolean",
> +    "description": "Successful telemetry submission"
> +  },
> +  "TELEMETRY_INVALID_PING_TYPE_SUBMITTED": {
> +    "alert_emails": ["[e-mail]"],

You might want to create a new alert email alias for you & Alessio, e.g. [e-mail]

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +194,5 @@
>     * Depending on configuration, the ping will be sent to the server (immediately or later)
>     * and archived locally.
>     *
> +   * To identify the different pings and to be able to query them, each user needs to submit
> +   * a different ping type.

"each user needs to submit a different ping type"?

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3098f602db56

Comment 11

[Georg Fritzsche [:gfritzsche]]

Backout: https://hg.mozilla.org/integration/fx-team/rev/33d19a206cad

Comment 12

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> Backout: https://hg.mozilla.org/integration/fx-team/rev/33d19a206cad

... for debug xpcshell bustage:
https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=3098f602db56
https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=f5d4f0d1fed9

Comment 13

[Georg Fritzsche [:gfritzsche]]

Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=a74157e2190b

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8602860 [details] [diff] [review]
Telemetry should reject external pings with invalid types

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8602860 [details] [diff] [review]
Telemetry should reject external pings with invalid types

Unified telemetry is an important new feature. It is blocking some other projects. Taking it.

Comment 17

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ccf0bdbac4eb