Closed Bug 1377340 Opened 3 years ago Closed 3 years ago

NETWORK_RACE_CACHE_WITH_NETWORK_USAGE should separate delayed and non-delayed racing

Categories

(Core :: Networking: Cache, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: michal, Assigned: michal)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(2 files, 1 obsolete file)

No description provided.
Attached patch patch (obsolete) — Splinter Review
Attachment #8882494 - Flags: review?(valentin.gosu)
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.
Attachment #8882494 - Flags: review?(valentin.gosu) → review+
Keywords: checkin-needed
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
Flags: needinfo?(michal.novotny)
Attached patch updated patchSplinter Review
Attachment #8882494 - Attachment is obsolete: true
Flags: needinfo?(michal.novotny)
Attachment #8883619 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7288e99df23
NETWORK_RACE_CACHE_WITH_NETWORK_USAGE should separate delayed and non-delayed racing. r=valentin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b7288e99df23
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Did this data collection change receive Data Collection Review? https://wiki.mozilla.org/Firefox/Data_Collection
Flags: needinfo?(michal.novotny)
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?
Flags: needinfo?(michal.novotny)
Attachment #8883619 - Flags: review?(benjamin)
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.
Attachment #8883619 - Flags: review?(benjamin) → review-
(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?
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8884331 - Flags: review?(valentin.gosu)
Attachment #8884331 - Flags: review?(benjamin)
Comment on attachment 8884331 [details] [diff] [review]
fix for an already landed patch

data-r=me
Attachment #8884331 - Flags: review?(benjamin) → review+
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!
Attachment #8884331 - Flags: review?(valentin.gosu) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0bfaa80673e
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Depends on: 1382852
You need to log in before you can comment on or make changes to this bug.