1377340 - NETWORK_RACE_CACHE_WITH_NETWORK_USAGE should separate delayed and non-delayed racing

Status: RESOLVED FIXED

(Core :: Networking: Cache, enhancement)

Comment 1

[Michal Novotny [:michal]]

Attachment: patch

Comment 2

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

Comment on attachment 8882494 [details] [diff] [review]
patch

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

Looks good. Not sure if we should land it until the shield study is completed.

Comment 3

[Carsten Book [:Tomcat]]

has problems to apply:

Hunk #3 succeeded at 9242 with fuzz 2 (offset 18 lines).
Hunk #4 succeeded at 9258 with fuzz 2 (offset 12 lines).
1 out of 4 hunks FAILED -- saving rejects to file netwerk/protocol/http/nsHttpChannel.cpp.rej
patching file netwerk/protocol/http/nsHttpChannel.h
Hunk #1 FAILED at 707
1 out of 1 hunks FAILED -- saving rejects to file netwerk/protocol/http/nsHttpChannel.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh diff-bug1377340-v1.patch

Comment 4

[Michal Novotny [:michal]]

Attachment: updated patch

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7288e99df23
NETWORK_RACE_CACHE_WITH_NETWORK_USAGE should separate delayed and non-delayed racing. r=valentin

Comment 6

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

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

Comment 7

[Chris H-C :chutten]

Did this data collection change receive Data Collection Review? https://wiki.mozilla.org/Firefox/Data_Collection

Comment 8

[Michal Novotny [:michal]]

Comment on attachment 8883619 [details] [diff] [review]
updated patch

(In reply to Chris H-C :chutten from comment #7)
> Did this data collection change receive Data Collection Review?
> https://wiki.mozilla.org/Firefox/Data_Collection

No, does really this minor change need data review?

Comment 9

[Benjamin Smedberg]

Comment on attachment 8883619 [details] [diff] [review]
updated patch

Yes, all changes should be reviewed.

In this case, I'm afraid you may have broken histogram aggregation for this value: you can't add new buckets to most histograms after it has been deployed (categorical histograms are an exception). Some people add extra buckets at first to add new values later, but since that didn't happen here.

Normally you would need to rename it (..NETWORK_USAGE2 or somesuch) to preserve aggregation.

I don't have good advice now that this has landed. If this hasn't hit a nightly yet, it would be best to back it out immediately. Otherwise please work with the data engineering team so we don't break all of t.m.o.

Comment 10

[Michal Novotny [:michal]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #9)
> I don't have good advice now that this has landed. If this hasn't hit a
> nightly yet, it would be best to back it out immediately. Otherwise please
> work with the data engineering team so we don't break all of t.m.o.

It has landed 2 days ago and I can see some data on t.m.o, so it's too late to back it out. IIUC, this could break only data for this probe, or do you mean it could break other probes too?

Comment 11

[Chris H-C :chutten]

Backing it out is sadly no longer an option. The aggregator has already received data from the new histogram.

Luckily this should only break this probe, not all probes. What happens, in broad strokes, is that in one build it had N buckets. Now it has more. These are not directly comparable or aggregatable, and we'll be seeing both types coming in from now on.

According to :frank, this might still somehow accidentally work. However, the safest path at this point is to add a renamed, versioned form of this probe to start recording things from this point on. (This might be a good opportunity to rework it as a  categorical histogram[1] as it has some nice properties unavailable to enumerated histograms)

So:
1) New histogram (call it ...NETWORK_USAGE2 or somesuch) in Histograms.json. Consider categorical.
2) Use it instead of ...NETWORK_USAGE in the C++ code.
3) f?/r? a Data Peer for Data Collection review

I apologize for not having the tooling to catch this before it reached the aggregator. Our documentation[2] is complete, but our error checking is somewhat less so. Please do not hesitate to ni?/f?/r? me or another Telemetry person to help out.

[1]: http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/histograms.html#choosing-a-histogram-type
[2]: http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/index.html

Comment 12

[Michal Novotny [:michal]]

Attachment: fix for an already landed patch

Sorry for messing this up. This patch renames the probe and changes it to categorical type.

Valentin, please check the if statements properly because it is easy to make a mistake there. IMO, it couldn't happen in ReportRcwnStats that (firstResponseRequest != mCachePump && !isFromNet) so I removed firstResponseRequest argument. But please double check it just in case I'm wrong.

Comment 13

[Benjamin Smedberg]

Comment on attachment 8884331 [details] [diff] [review]
fix for an already landed patch

data-r=me

Comment 14

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

Comment on attachment 8884331 [details] [diff] [review]
fix for an already landed patch

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

Looks good. You're right about the argument, it didn't bring any value there.
Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0bfaa80673e
NETWORK_RACE_CACHE_WITH_NETWORK_USAGE should separate delayed and non-delayed racing. r=valentin, data-r=bsmedberg

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/e0bfaa80673e

Comment 17

[Ekanan Ketunuti]

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