Georg Fritzsche [:gfritzsche] Assignee
	Description • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 1 • 9 years ago

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.

	Mark Reid [:mreid]
	Comment 2 • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 3 • 9 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 4 • 9 years ago

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).

	Mark Reid [:mreid]
	Comment 5 • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 6 • 9 years ago

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

	Benjamin Smedberg
	Comment 7 • 9 years ago

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.

	Vladan Djeric (:vladan)
	Comment 8 • 9 years ago

(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

	Vladan Djeric (:vladan)
	Comment 9 • 9 years ago

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": ["telemetry-alerts@mozilla.com"],

You don't need to set telemetry-alerts@mozilla.com, 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 dev-telemetry-alerts@lists.mozilla.org, the alerts get sent FROM telemetry-alerts@mozilla.com

@@ +4403,5 @@
> +    "kind": "boolean",
> +    "description": "Successful telemetry submission"
> +  },
> +  "TELEMETRY_INVALID_PING_TYPE_SUBMITTED": {
> +    "alert_emails": ["telemetry-alerts@mozilla.com"],

You might want to create a new alert email alias for you & Alessio, e.g. telemetry-client-dev@mozilla.com

::: 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"?

	Pulsebot
	Comment 10 • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 11 • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 12 • 9 years ago

(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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 13 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 15 • 9 years ago

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.

	Sylvestre Ledru [:Sylvestre]
	Comment 16 • 9 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 17 • 9 years ago

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