Closed Bug 1186492 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Iteration:
42.3 - Aug 10
Tracking Status
firefox40 --- wontfix
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [unifiedTelemetry] [uplift4])

Attachments

(2 files, 1 obsolete file)

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
Blocks: 1120356
Whiteboard: [unifiedTelemetry]
Can we expand on what this information is to be used for?
Shouldn't we track the different failures on the server-side?
(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?
Flags: needinfo?(alessio.placitelli)
(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.
Flags: needinfo?(alessio.placitelli)
Summary: Add a probe to track server errors on the client → Add a probe to track client-side ping eviction due to server errors
Assignee: nobody → alessio.placitelli
Assignee: alessio.placitelli → nobody
Assignee: nobody → alessio.placitelli
Whiteboard: [unifiedTelemetry] → [unifiedTelemetry] [uplift4]
Attached patch bug1186492.patch (obsolete) — Splinter Review
Attachment #8640603 - Flags: review?(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": ["telemetry-client-dev@mozilla.com"],
> +    "expires_in_version": "never",
> +    "kind": "count",
> +    "description": "Number of Telemetry pings files evicted due to server errors (4XX HTTP code received)"

Nit: "ping files"
Attachment #8640603 - Flags: review?(gfritzsche) → review+
Attachment #8640603 - Attachment is obsolete: true
Attachment #8641059 - Flags: review+
Attachment #8641060 - Flags: review?(gfritzsche)
Attachment #8641060 - Flags: review?(gfritzsche) → review+
Iteration: --- → 42.3 - Aug 10
https://hg.mozilla.org/mozilla-central/rev/54d3c0137056
https://hg.mozilla.org/mozilla-central/rev/5ad2d34c58b1
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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.
Attachment #8641059 - Flags: approval-mozilla-aurora?
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.
Attachment #8641060 - Flags: approval-mozilla-aurora?
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.
Attachment #8641060 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
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.
Attachment #8641059 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.