Closed
Bug 1377340
Opened 7 years ago
Closed 7 years ago
NETWORK_RACE_CACHE_WITH_NETWORK_USAGE should separate delayed and non-delayed racing
Categories
(Core :: Networking: Cache, enhancement)
Core
Networking: Cache
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)
6.72 KB,
patch
|
michal
:
review+
benjamin
:
review-
|
Details | Diff | Splinter Review |
7.29 KB,
patch
|
valentin
:
review+
benjamin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 2•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 3•7 years ago
|
||
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)
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8882494 -
Attachment is obsolete: true
Flags: needinfo?(michal.novotny)
Attachment #8883619 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
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
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7288e99df23
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 7•7 years ago
|
||
Did this data collection change receive Data Collection Review? https://wiki.mozilla.org/Firefox/Data_Collection
Flags: needinfo?(michal.novotny)
Assignee | ||
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
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-
Assignee | ||
Comment 10•7 years ago
|
||
(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•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 12•7 years ago
|
||
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 13•7 years ago
|
||
Comment on attachment 8884331 [details] [diff] [review] fix for an already landed patch data-r=me
Attachment #8884331 -
Flags: review?(benjamin) → review+
Comment 14•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 15•7 years ago
|
||
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
![]() |
||
Comment 16•7 years ago
|
||
bugherder landing |
https://hg.mozilla.org/integration/autoland/rev/e0bfaa80673e
![]() |
||
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e0bfaa80673e
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•