1272345 - Gather Telemetry on URL redirects

Status: RESOLVED FIXED

(Core :: Networking, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1272345_telemetry_url_redirects.patch

Valentin, I based my patch on HTTP_SCHEME_UPGRADE since it seems similar. I am not sure if we should include more status codes or just redirects. I personally only care about redirects, but let me know what you think.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8751767 [details] [diff] [review]
bug_1272345_telemetry_url_redirects.patch

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1611,5 @@
>          bool saw_quic = (alt_protocol && PL_strstr(alt_protocol, "quic")) ? 1 : 0;
>          Telemetry::Accumulate(Telemetry::HTTP_SAW_QUIC_ALT_PROTOCOL, saw_quic);
> +
> +        // Gather data on how many URLS get redirected
> +        switch(httpStatus) {

Make this conditional on if Telemetry is enabled (similar to nsIOService).

@@ +1628,5 @@
> +            case 308:
> +                Telemetry::Accumulate(Telemetry::HTTP_RESPONSE_STATUS_CODE, 4);
> +                break;
> +            default:
> +                break;

Patrick suggests also adding buckets for: 304, 400, 401, 403, 404, 500 and one for "other"

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Valentin Gosu [:valentin] from comment #2)
> Make this conditional on if Telemetry is enabled (similar to nsIOService).

It's already inside |if (gHttpHandler->IsTelemetryEnabled()) {|

> Patrick suggests also adding buckets for: 304, 400, 401, 403, 404, 500 and
> one for "other"

Makes sense, will do.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1272345_telemetry_url_redirects.patch

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8751772 [details] [diff] [review]
bug_1272345_telemetry_url_redirects.patch

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

r=me with a minor nit. Thanks!

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1611,5 @@
>          bool saw_quic = (alt_protocol && PL_strstr(alt_protocol, "quic")) ? 1 : 0;
>          Telemetry::Accumulate(Telemetry::HTTP_SAW_QUIC_ALT_PROTOCOL, saw_quic);
> +
> +        // Gather data on how many URLS get redirected
> +        switch(httpStatus) {

nit: space before (

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5ab82ceff1

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb5ab82ceff1