1186492 - Add a probe to track client-side ping eviction due to server errors

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Alessio Placitelli [:Dexter]]

We should track how many times TelemetrySend fails to send pings due to 4xx, 5xx or other errors [1].

[1] - https://hg.mozilla.org/mozilla-central/annotate/8e5c888d0d89/toolkit/components/telemetry/TelemetrySend.jsm#l880

Comment 1

[Georg Fritzsche [:gfritzsche]]

Can we expand on what this information is to be used for?
Shouldn't we track the different failures on the server-side?

Comment 2

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] [away until july 22] from comment #1)
> Can we expand on what this information is to be used for?
> Shouldn't we track the different failures on the server-side?

Or is this specifically about tracking how often we delete pings due to those circumstances?

Comment 3

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] [away until july 22] from comment #2)
> Or is this specifically about tracking how often we delete pings due to
> those circumstances?

Yes, this is to track that, as it could affect session chaining.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: bug1186492.patch

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8640603 [details] [diff] [review]
bug1186492.patch

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

This looks ok, with the below resolved.
We need to add test-coverage for this though, i assume its not too hard to make the test server return a 4xx code.

::: toolkit/components/telemetry/Histograms.json
@@ +4744,5 @@
>      "kind": "count",
>      "keyed": true,
>      "description": "Count of individual invalid ping types that were submitted to Telemetry."
>    },
> +  "TELEMETRY_SERVER_ERRORS_PING_FILES_EVICTED": {

TELEMETRY_PING_EVICTED_FOR_SERVER_ERRORS ?

@@ +4748,5 @@
> +  "TELEMETRY_SERVER_ERRORS_PING_FILES_EVICTED": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",
> +    "kind": "count",
> +    "description": "Number of Telemetry pings files evicted due to server errors (4XX HTTP code received)"

Nit: "ping files"

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add the probe

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage

Comment 8

[Alessio Placitelli [:Dexter]]

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

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/54d3c0137056
https://hg.mozilla.org/integration/fx-team/rev/5ad2d34c58b1

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/54d3c0137056
https://hg.mozilla.org/mozilla-central/rev/5ad2d34c58b1

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8641059 [details] [diff] [review]
part 1 - Add the probe

Approval Request Comment
[Feature/regressing bug #]:
Telemetry Unification
[User impact if declined]:
This adds diagnostics for when we discard pings because the Telemetry server returned a 4xx status code - we need this for validating the incoming data and detecting issues.
It is part of a mostly fixup & diagnostic uplift batch for 41: http://bit.ly/1LYhA16
[Describe test coverage new/current, TreeHerder]:
We have automated test-coverage, it baked on Nightly, we are monitoring it.
[Risks and why]:
Low-risk, we haven't seen anything crop up on Nightly here.
[String/UUID change made/needed]:
None.

Comment 12

[Georg Fritzsche [:gfritzsche]]

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

Approval Request Comment
[Feature/regressing bug #]:
Telemetry Unification
[User impact if declined]:
This adds tests for the probe.
It is part of a mostly fixup & diagnostic uplift batch for 41: http://bit.ly/1LYhA16
[Describe test coverage new/current, TreeHerder]:
We have automated test-coverage, it baked on Nightly, we are monitoring it.
[Risks and why]:
Low-risk, we haven't seen anything crop up on Nightly here.
[String/UUID change made/needed]:
None.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

[Triage Comment]
More automated tests are always a good thing. Approved for uplift to m-b.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8641059 [details] [diff] [review]
part 1 - Add the probe

[Triage Comment]
UT is targeted for FF41. This patch seems simple, adds a probe and has automated tests. Approved for uplift to m-b.

Comment 15

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-beta/rev/125d7c0718e5
https://hg.mozilla.org/releases/mozilla-beta/rev/5650d1a8106f