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

RESOLVED FIXED in Firefox 41

Status

()

Toolkit
Telemetry
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 wontfix, firefox41 fixed, firefox42 fixed)

Details

(Whiteboard: [unifiedTelemetry] [uplift4])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 3

2 years ago
(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)
Blocks: 1185123
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)

Updated

2 years ago
Assignee: nobody → alessio.placitelli
(Assignee)

Updated

2 years ago
Assignee: alessio.placitelli → nobody
(Assignee)

Updated

2 years ago
Assignee: nobody → alessio.placitelli
Whiteboard: [unifiedTelemetry] → [unifiedTelemetry] [uplift4]
(Assignee)

Comment 4

2 years ago
Created attachment 8640603 [details] [diff] [review]
bug1186492.patch
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+
(Assignee)

Comment 6

2 years ago
Created attachment 8641059 [details] [diff] [review]
part 1 - Add the probe
Attachment #8640603 - Attachment is obsolete: true
Attachment #8641059 - Flags: review+
(Assignee)

Comment 7

2 years ago
Created attachment 8641060 [details] [diff] [review]
part 2 - Add test coverage
Attachment #8641060 - Flags: review?(gfritzsche)
Attachment #8641060 - Flags: review?(gfritzsche) → review+
Iteration: --- → 42.3 - Aug 10
(Assignee)

Comment 8

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

Comment 9

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/54d3c0137056
https://hg.mozilla.org/integration/fx-team/rev/5ad2d34c58b1
https://hg.mozilla.org/mozilla-central/rev/54d3c0137056
https://hg.mozilla.org/mozilla-central/rev/5ad2d34c58b1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
status-firefox40: --- → wontfix
status-firefox41: --- → affected
Blocks: 1191757
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+
https://hg.mozilla.org/releases/mozilla-beta/rev/125d7c0718e5
https://hg.mozilla.org/releases/mozilla-beta/rev/5650d1a8106f
status-firefox41: affected → fixed
You need to log in before you can comment on or make changes to this bug.