Telemetry for FxAccountsClient includes query params in the key.

RESOLVED FIXED in Firefox 46

Status

()

P1
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: markh, Assigned: lina)

Tracking

unspecified
Firefox 46
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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+
Priority: -- → P1
Assignee: nobody → kcambridge
(Assignee)

Comment 1

3 years ago
This probe was removed in bug 1236383.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 2

3 years ago
We should make sure other paths omit the query params, though.
(Reporter)

Comment 3

3 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

3 years ago
Created attachment 8708075 [details]
MozReview Request: Bug 1233298 - Remove query params from the `FXA_HAWK_ERRORS` probe. r?markh

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

3 years ago
Status: REOPENED → ASSIGNED
(Reporter)

Updated

3 years ago
Attachment #8708075 - Flags: review?(markh)
(Reporter)

Comment 5

3 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

3 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

3 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

3 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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f2d228dc6d9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Updated

a year ago
Product: Core → Firefox
Target Milestone: mozilla46 → Firefox 46
You need to log in before you can comment on or make changes to this bug.