1390683 - HTTP_*_COMPLETE_LOAD probes are used in nsLoadGroup::TelemetryReportChannel when using new cache

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Michal Novotny [:michal]]

_V2 probe should be used when using new cache at http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/netwerk/base/nsLoadGroup.cpp#989

Comment 1

[Michal Novotny [:michal]]

Attachment: fix

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8897619 [details] [diff] [review]
fix

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

Yes!  And please duplicate bug 1380370 to this one.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8897619 [details] [diff] [review]
fix

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

::: netwerk/base/nsLoadGroup.cpp
@@ +988,3 @@
>          if (!CacheObserver::UseNewCache()) {                                   \
>              Telemetry::AccumulateTimeDelta(                                    \
> +                Telemetry::HTTP_##prefix##_COMPLETE_LOAD,                      \

hmm... I think it might be good to rename this to HTTP_##prefix##_COMPLETE_LOAD_V1 so that we don't have an intermix with the previous data.

Comment 5

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> ::: netwerk/base/nsLoadGroup.cpp
> @@ +988,3 @@
> >          if (!CacheObserver::UseNewCache()) {                                   \
> >              Telemetry::AccumulateTimeDelta(                                    \
> > +                Telemetry::HTTP_##prefix##_COMPLETE_LOAD,                      \
> 
> hmm... I think it might be good to rename this to
> HTTP_##prefix##_COMPLETE_LOAD_V1 so that we don't have an intermix with the
> previous data.

But in this case we should replace V2 with a new probe too. Given how many reports we have for HTTP_*_COMPLETE_LOAD_NET and HTTP_*_COMPLETE_LOAD_CACHED, we can simply assume that all reports for HTTP_##prefix##_COMPLETE_LOAD should be in fact for HTTP_##prefix##_COMPLETE_LOAD_V2. I'll delete the old probes in bug 1382688 anyway, so I don't want to go through data review for a new probe which will be removed soon.

Comment 6

[Honza Bambas (:mayhemer)]

Good with me, thanks.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0bdef13ed8c
HTTP_*_COMPLETE_LOAD probes are used in nsLoadGroup::TelemetryReportChannel when using new cache, r=honzab

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Chris H-C :chutten]

This bug may have contributed to a sudden change in the Telemetry probe HTTP_PAGE_COMPLETE_LOAD_V2[1] which seems to have occurred in Nightly 20170817[2][3].

There was a general decrease and a switch from a modal to a bimodal distribution, possibly due to an increase in the number of samples recorded to the probe.

Is this an improvement? A regression?

Is this intentional? Is this expected?

Is this probe still measuring something useful?

[1]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/861/alerts/?from=2017-08-17&to=2017-08-17
[2]: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6ebc251bd288c268b020815025b05854ccde5c08&tochange=932388b8c22c9775264e543697ce918415db9e23
[3]: https://mzl.la/2gNs5N9

Comment 10

[Michal Novotny [:michal]]

(In reply to Chris H-C :chutten from comment #9)
> This bug may have contributed to a sudden change in the Telemetry probe
> HTTP_PAGE_COMPLETE_LOAD_V2[1] which seems to have occurred in Nightly
> 20170817[2][3].
> 
> There was a general decrease and a switch from a modal to a bimodal
> distribution, possibly due to an increase in the number of samples recorded
> to the probe.
> 
> Is this an improvement? A regression?

Neither improvement, nor regression. We reported part of the data incorrectly to HTTP_PAGE_COMPLETE_LOAD. HTTP_PAGE_COMPLETE_LOAD_V2 now contains correctly all data, i.e. it's a merge of what was previously in two histograms:

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-08-08&keys=__none__!__none__!__none__&max_channel_version=beta%252F56&measure=HTTP_PAGE_COMPLETE_LOAD_V2&min_channel_version=beta%252F53&processType=*&product=Firefox&sanitize=0&sort_keys=submissions&start_date=2017-08-08&table=0&trim=1&use_submission_date=0

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-08-08&keys=__none__!__none__!__none__&max_channel_version=beta%252F56&measure=HTTP_PAGE_COMPLETE_LOAD&min_channel_version=nightly%252F57&processType=*&product=Firefox&sanitize=0&sort_keys=submissions&start_date=2017-08-08&table=0&trim=1&use_submission_date=0

> Is this intentional? Is this expected?

Yes

> Is this probe still measuring something useful?

Yes, it measures still the same.