If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

NETWORK_RACE_CACHE_WITH_NETWORK_USAGE should separate delayed and non-delayed racing

RESOLVED FIXED in Firefox 56

Status

()

Core
Networking: Cache
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: michal, Assigned: michal)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 months ago
Created attachment 8882494 [details] [diff] [review]
patch
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+
(Assignee)

Updated

3 months ago
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)

Updated

3 months ago
Keywords: checkin-needed
(Assignee)

Comment 4

3 months ago
Created attachment 8883619 [details] [diff] [review]
updated patch
Attachment #8882494 - Attachment is obsolete: true
Attachment #8883619 - Flags: review+
Flags: needinfo?(michal.novotny)
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 5

3 months ago
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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7288e99df23
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 7

3 months ago
Did this data collection change receive Data Collection Review? https://wiki.mozilla.org/Firefox/Data_Collection
Flags: needinfo?(michal.novotny)
(Assignee)

Comment 8

3 months 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

3 months 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

3 months 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

3 months 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

3 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

3 months ago
Created attachment 8884331 [details] [diff] [review]
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.
Attachment #8884331 - Flags: review?(valentin.gosu)
Attachment #8884331 - Flags: review?(benjamin)

Comment 13

3 months ago
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+
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 15

3 months 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
https://hg.mozilla.org/integration/autoland/rev/e0bfaa80673e

Comment 17

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0bfaa80673e
Status: REOPENED → RESOLVED
Last Resolved: 3 months ago3 months ago
Resolution: --- → FIXED
(Assignee)

Updated

2 months ago
Depends on: 1382852
You need to log in before you can comment on or make changes to this bug.