Closed Bug 1380880 Opened 2 years ago Closed 2 years ago

Keyed histograms don't distinguish process type

Categories

(Toolkit :: Telemetry, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
Suppose you have two a keyed histogram named "KEYED" and both the parent and the child add to it with the key "X". Ideally, we should generate different data for the parent and the child for "KEYED" (i.e., separate histograms, each with a count=1 in the histogram for "X"). But we're actually combining them, so we would report count=2 for "X" for both the parent and the child.

The bug happens because:
1. The mName field is the same for both the parent and the child KeyedHistograms, as one can see here:
http://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/toolkit/components/telemetry/TelemetryHistogram.cpp#1860-1883

2. When we look up a histogram, we rely on a unique identifier constructed as follows:
http://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/toolkit/components/telemetry/TelemetryHistogram.cpp#943-951

Note that the ID does not include the process type in any way! internal_HistogramGet will return the same histogram if you pass it the same name, so we will use the same data structure for the parent and the child.

This patch fixes the problem by adding an mProcessType field and using it to generate the ID.
Attachment #8886431 - Flags: review?(gfritzsche)
Assignee: nobody → wmccloskey
Flags: needinfo?(wmccloskey)
Note that we are doing a fundamental refactor of how histograms are stored in bug 1366294, which is close to landing.
This should resolve any problems that result from the current storage model (that tries to encode the histogram information into a string).

Is there a specific test case or example histogram for this?

Chris, can you take a look here if:
- Any test-cases show/reproduce this problem.
- If we have test coverage on this.

If there is a bug here we'd have to check into how/if historic data is affected.
Flags: needinfo?(chutten)
Comment on attachment 8886431 [details] [diff] [review]
patch

Cancelling review as bug 1366294 should supersede this.
Attachment #8886431 - Flags: review?(gfritzsche)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> Are keyed histograms not reported in the child payloads section of the ping?

They are. It's just that the data is wrong. It's wrong in the parent too because of the merging behavior I described. This doesn't hit us too often because it only happens when there's a keyed histogram where the parent and child use the same key. But it's a massive problem for histograms like MAIN_THREAD_RUNNABLE_MS, which is the histogram we use to direct all the Quantum DOM labeling work.
Flags: needinfo?(wmccloskey)
Comment on attachment 8886431 [details] [diff] [review]
patch

Re-requesting review. This is a really significant problem for Quantum DOM and I want to get this patch landed ASAP. Bug 1366294 probably will fix the problem, but it looks like a major refactor. Even if it lands today, it might take time to stabilize. I don't want to risk it getting backed out, resulting in us losing more data.

I can write a test for this today. It's very easy to exhibit the bad behavior.
Attachment #8886431 - Flags: review?(gfritzsche)
Comment on attachment 8886431 [details] [diff] [review]
patch

Understood - Chris, can you review?

Can we get a test case into test_ChildHistograms.js?
(In a follow-up if that helps with unblocking)
Attachment #8886431 - Flags: review?(gfritzsche) → review?(chutten)
Comment on attachment 8886431 [details] [diff] [review]
patch

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

Needs a test to make sure

1) It fixes things
2) It still puts the keyed histograms in the right place and with the right names in the payloads.
Attachment #8886431 - Flags: review?(chutten) → review-
Ugh, that ended up a little harsh sounding.

What I mean is, let's write a test before we land this. The code looks good, but given how long we missed this so far, I'm no longer so confident in my ability to see good code :S

Should just be a matter of adding a few lines to test_ChildHistograms.js
Flags: needinfo?(chutten)
Attached patch patch with testSplinter Review
Attachment #8886431 - Attachment is obsolete: true
Attachment #8886652 - Flags: review?(chutten)
Attachment #8886652 - Flags: review?(chutten) → review+
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c1415579a9b
Use process type to distinguish keyed histograms (r=chutten)
https://hg.mozilla.org/mozilla-central/rev/5c1415579a9b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1381516
You need to log in before you can comment on or make changes to this bug.