1380880 - Keyed histograms don't distinguish process type

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

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.

Comment 1

[Benjamin Smedberg]

Are keyed histograms not reported in the child payloads section of the ping?

It appears that on t.m.o you can distinguish processes for keyed histograms using the normal process selector: https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-07-12&keys=Shockwave%2520Flash26.0.0.131!Shockwave%2520Flash26.0.0.126!Shockwave%2520Flash25.0.0.171!Shockwave%2520Flash25.0.0.171&max_channel_version=nightly%252F56&measure=BLOCKED_ON_PLUGIN_INSTANCE_INIT_MS&min_channel_version=null&processType=parent&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-06-12&table=0&trim=1&use_submission_date=0

Comment 2

[Georg Fritzsche [:gfritzsche]]

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.

Comment 3

[Georg Fritzsche [:gfritzsche]]

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

Cancelling review as bug 1366294 should supersede this.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Georg Fritzsche [: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)

Comment 7

[Chris H-C :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.

Comment 8

[Chris H-C :chutten]

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

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch with test

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c1415579a9b
Use process type to distinguish keyed histograms (r=chutten)

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/5c1415579a9b