Closed
Bug 1233298
Opened 7 years ago
Closed 6 years ago
Telemetry for FxAccountsClient includes query params in the key.
Categories
(Firefox :: Firefox Accounts, defect, P1)
Firefox
Firefox Accounts
Tracking
()
RESOLVED
FIXED
Firefox 46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: markh, Assigned: lina)
References
Details
Attachments
(1 file)
If you look at telemetry dashboards for FXA_SERVER_ERROR, at least one "path" is reported as /account/status?uid=..., which implies all telemetry reported by this module will include query params in the "key", making some analysis somewhere between difficult and impossible :) Fortunately this seems rare, and it may be we remove other otherwise tweak these probes, but it's worth having this in a bug.
Flags: firefox-backlog+
Updated•7 years ago
|
Priority: -- → P1
Updated•7 years ago
|
Assignee: nobody → kcambridge
Assignee | ||
Comment 1•7 years ago
|
||
This probe was removed in bug 1236383.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Assignee | ||
Comment 2•7 years ago
|
||
We should make sure other paths omit the query params, though.
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to Kit Cambridge [:kitcambridge] from comment #1) > This probe was removed in bug 1236383. > We should make sure other paths omit the query params, though. Yeah - that bug is leaving one probe using the path as a key - it probably never has query params today but they could slip in tomorrow, so I think we should still sanitize it.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee | ||
Comment 4•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/30961/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30961/
Attachment #8708075 -
Flags: review?(markh)
Assignee | ||
Updated•6 years ago
|
Status: REOPENED → ASSIGNED
Reporter | ||
Updated•6 years ago
|
Attachment #8708075 -
Flags: review?(markh)
Reporter | ||
Comment 5•6 years ago
|
||
Comment on attachment 8708075 [details] MozReview Request: Bug 1233298 - Remove query params from the `FXA_HAWK_ERRORS` probe. r?markh https://reviewboard.mozilla.org/r/30961/#review28589 ::: services/fxaccounts/FxAccountsClient.jsm:417 (Diff revision 1) > - "FXA_HAWK_ERRORS").add(path); > + "FXA_HAWK_ERRORS").add(uri.filePath); I don't really see the value in a new public method on hawk just for this one probe. We know |path| must be a string here, so can we consider (say) a regex that strips anything past a '?' or '#'? It looks like it would then be a 1 line change. ::: services/fxaccounts/tests/xpcshell/test_client.js:655 (Diff revision 1) > + service: "sync", why is this change necessary as part of this patch?
Assignee | ||
Comment 6•6 years ago
|
||
https://reviewboard.mozilla.org/r/30961/#review28591 ::: services/fxaccounts/FxAccountsClient.jsm:417 (Diff revision 1) > - "FXA_HAWK_ERRORS").add(path); > + "FXA_HAWK_ERRORS").add(uri.filePath); Works for me. I considered using a regex, but `RESTRequest` parses the string into an `nsIURI`, so I thought we might as well convert early. ::: services/fxaccounts/tests/xpcshell/test_client.js:655 (Diff revision 1) > + service: "sync", This adds a `service` query param to the path.
Assignee | ||
Comment 7•6 years ago
|
||
Comment on attachment 8708075 [details] MozReview Request: Bug 1233298 - Remove query params from the `FXA_HAWK_ERRORS` probe. r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30961/diff/1-2/
Attachment #8708075 -
Flags: review?(markh)
Reporter | ||
Comment 8•6 years ago
|
||
Comment on attachment 8708075 [details] MozReview Request: Bug 1233298 - Remove query params from the `FXA_HAWK_ERRORS` probe. r?markh https://reviewboard.mozilla.org/r/30961/#review28595 Awesome, thanks!
Attachment #8708075 -
Flags: review?(markh) → review+
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3f2d228dc6d9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 6 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•5 years ago
|
Product: Core → Firefox
Updated•5 years ago
|
Target Milestone: mozilla46 → Firefox 46
You need to log in
before you can comment on or make changes to this bug.
Description
•