nsITelemetry.histogramSnapshots should not return keyed histograms

RESOLVED FIXED in Firefox 55

Status

()

defect
P3
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Dexter, Assigned: gmoore, Mentored)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(3 attachments, 4 obsolete attachments)

Keyed histograms are displayed in the "Histograms" section of about:telemetry as well as in the "Keyed Histograms" section (e.g. "SEARCH_COUNTS#google.searchbar").
nsITelemetry.histogramSnapshots returns all histograms. Instead it should filter out keyed histograms.
Note that this is not an issue with the actual outgoing Telemetry pings, as  that implementation happens to filter out keyed histograms.
Mentor: gfritzsche
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [lang=c++]
The histogramSnapshots implementation is here:
https://hg.mozilla.org/mozilla-central/annotate/fa91879c8428/toolkit/components/telemetry/Telemetry.cpp#l2117

This should filter out keyed histograms.

IdentifyCorruptHistograms() shows how to get the enum id. With that we can check |gHistograms[id].keyed| and filter them out.
Vladan, do you think the keyed histograms should affect the global corruption-statistics for histograms?
I.e., should we filter out keyed histograms in GetHistogramSnapshots() before or after IdentifyCorruptHistograms()?
Flags: needinfo?(vdjeric)
Does IdentifyCorruptHistograms even work correctly for keyed-histograms right now? It uses gCorruptHistograms[Telemetry::HistogramCount] to store corruption state per histogram, but HistogramCount is the static number of histograms defined in Histograms.json.. How does it deal with a variable number of keyed histograms? I think this should be looked into.

To answer your question, I think we should filter on keyed/non-keyed in the "Get..Snapshots" functions first and then call IdentifyCorruptHistograms on the remaining histograms. That way each "Get..Snapshots" function would check that only the histograms it's about to return aren't corrupted.
Flags: needinfo?(vdjeric)
(In reply to Vladan Djeric (:vladan) from comment #4)
> Does IdentifyCorruptHistograms even work correctly for keyed-histograms
> right now?

Actually it doesn't do anything with them at all. For a keyed histogram with name "A" and used keys "B" and "C" we have histograms named "A#B" and "A#C".
We don't find an id for them here:
https://hg.mozilla.org/mozilla-central/annotate/38e4719e71af/toolkit/components/telemetry/Telemetry.cpp#l1944
... so we just skip them in IdentifyCorrupHistograms().

ni?me to setup proper mentored-bug advice.
Flags: needinfo?(gfritzsche)
Per bug 1122480 we don't show the keyed histograms anymore in the plain histogram section in about:telemetry (we now show the data directly from pings).
Summary: Keyed histograms should only be shown in keyed histograms section in about:telemetry → nsITelemetry.histogramSnapshots should not return keyed histograms
Georg,

I'm interested in working on this bug. Can you please give me more info on where to get started?
(In reply to Ryan Nath from comment #7)
> I'm interested in working on this bug. Can you please give me more info on
> where to get started?

Hi Ryan, sure!

We'll want to skip keyed histograms for reflection around here:
https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/components/telemetry/Telemetry.cpp#l2302

There is a check for whether a histogram is keyed here:
https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/components/telemetry/Telemetry.cpp#l2272

To do that check we need to get the |TelemetryHistogram| for the |Histogram*|, there is some examples of how to get that around in the tree:
You need to use TelemetryImpl::GetHistogramEnumId() with the histograms name to get the id, with that you can do the check above.

about:telemetry doesn't have that issue anymore, but for quick checking you can use the Scratchpad.
* Make sure chrome devtools are enabled
* tools -> web developer -> scratchpad
* switch the scratchpads environment to "browser"

Use something like the following and hit "inspect":
> let telemetry = Services.telemetry;
> let keyed = telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT");
> keyed.add("a", 1);
> Services.telemetry.histogramSnapshots

Filtering the properties you'll see TELEMETRY_TEST_KEYED_COUNT before the changes.

For testing you'll want:
* run the most relevant test with: mach xpcshell-test toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
* extend that test with a new test function around here: https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js#l874
* that function should make sure a keyed histogram was created like the snippet above
* and then check that the keyed histogram is not in the histogramSnapshots

After things work make sure this didn't break other Telemetry behavior by running:
mach xpcshell-test toolkit/components/telemetry/tests/unit/

... let me know if anything is unclear.
Flags: needinfo?(gfritzsche)
Any progress happening here?
Flags: needinfo?(gfritzsche)
Nothing happened here since comment 8.
Are you interested in taking on this bug?
Flags: needinfo?(gfritzsche)

Comment 11

4 years ago
Hello, I would like to work on this bug
I think this is a bit involved for a first bug, i'd recommend filtering e.g. here for "simple" / "good first" bugs: 
http://www.joshmatthews.net/bugsahoy/?unowned=1&simple=1
Assigning to Eric, who wanted to take a look :)
Assignee: nobody → hu.eric
(In reply to Georg Fritzsche [:gfritzsche] [away Jun 24 - Jul 3] from comment #8)
> We'll want to skip keyed histograms for reflection around here:
> https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/
> components/telemetry/Telemetry.cpp#l2302
> 
> There is a check for whether a histogram is keyed here:
> https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/
> components/telemetry/Telemetry.cpp#l2272
> 
> To do that check we need to get the |TelemetryHistogram| for the
> |Histogram*|, there is some examples of how to get that around in the tree:
> You need to use TelemetryImpl::GetHistogramEnumId() with the histograms name
> to get the id, with that you can do the check above.
> 
> about:telemetry doesn't have that issue anymore, but for quick checking you
> can use the Scratchpad.
> * Make sure chrome devtools are enabled
> * tools -> web developer -> scratchpad
> * switch the scratchpads environment to "browser"
> 
> Use something like the following and hit "inspect":
> > let telemetry = Services.telemetry;
> > let keyed = telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT");
> > keyed.add("a", 1);
> > Services.telemetry.histogramSnapshots
> 
> Filtering the properties you'll see TELEMETRY_TEST_KEYED_COUNT before the
> changes.

This is not correct, this shows up as "TELEMETRY_TEST_KEYED_COUNT#a" (because we store each entry in a keyed histogram in the format "HISTOGRAM_NAME#key").

Note that the serialization functions linked to in comment 8 moved from Telemetry.cpp to TelemetryHistogram.cpp, otherwise they should be fundamentally the same.
This is currently not actively worked on.
Assignee: hu.eric → nobody
Assignee

Comment 16

2 years ago
Hi, I am interested in working on this bug if no one else is and this is still an issue. It looks like there's already enough information to get started, but I just wanted to check if there was anything else I should know. 

Also, I don't have much experience with JavaScript, so I wanted to check if that would be a problem. Let me know, thanks.
Flags: needinfo?(gfritzsche)
Hi Gregory, no-one else is working on this currently.
This bug doesn't require special JavaScript or C++ knowledge: the changes required here are rather contained.

Comment 8 & comment 14 should have the relevant information.
The only thing that changed really is that the code was moved to TelemetryHistogram.cpp:
https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/toolkit/components/telemetry/TelemetryHistogram.cpp#2320

I would recommend to manually confirm the problem first in the browser before getting started.
Flags: needinfo?(gfritzsche)
Mentor: alessio.placitelli
(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> * run the most relevant test with: mach xpcshell-test
> toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
> * extend that test with a new test function around here:
> https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/
> components/telemetry/tests/unit/test_nsITelemetry.js#l874

Also, this test was renamed since then:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
Assignee

Comment 19

2 years ago
Okay cool, thanks for replying and for the information. 

(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> I would recommend to manually confirm the problem first in the browser
> before getting started.

> Filtering the properties you'll see TELEMETRY_TEST_KEYED_COUNT before the
> changes.
I'm not exactly sure what you mean by see "TELEMETRY_TEST_KEYED_COUNT" (or "TELEMETRY_TEST_KEYED_COUNT#a" rather, from comment 14) before the changes. Do you just mean before we fix the problem? I can run the JavaScript code and I get the output shown in the screenshot I attached. I do see the TELEMETRY_TEST_KEYED_COUNT#a property, so I'm guessing the problem is still there, but I just wanted to make sure I was checking for it correctly. Let me know, thanks.
Flags: needinfo?(gfritzsche)
(In reply to Gregory Moore [:gmoore] from comment #19)
> Created attachment 8838239 [details]
> Screenshot of ScratchPad output.
> 
> Okay cool, thanks for replying and for the information. 
> 
> (In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> > I would recommend to manually confirm the problem first in the browser
> > before getting started.
> 
> > Filtering the properties you'll see TELEMETRY_TEST_KEYED_COUNT before the
> > changes.
> I'm not exactly sure what you mean by see "TELEMETRY_TEST_KEYED_COUNT" (or
> "TELEMETRY_TEST_KEYED_COUNT#a" rather, from comment 14) before the changes.
> Do you just mean before we fix the problem? I can run the JavaScript code
> and I get the output shown in the screenshot I attached. I do see the
> TELEMETRY_TEST_KEYED_COUNT#a property, so I'm guessing the problem is still
> there, but I just wanted to make sure I was checking for it correctly. Let
> me know, thanks.

Exactly, we should still see this before we the problem.
It means that the problem still exists.
Flags: needinfo?(gfritzsche)
Assignee

Comment 21

2 years ago
Posted patch Initial version of patch. (obsolete) — Splinter Review
Okay, cool, thanks. I finished an initial version of the patch, and it seems to fix the problem. test_TelemetryHistograms.js as well as all of the other tests in toolkit/components/telemetry/tests/unit/ are currently passing, and I no longer see the keyed histograms in ScratchPad.

It seemed like at first I couldn't ever get the "TELEMETRY_TEST_KEYED_COUNT#a" property to be returned by Services.telemetry.histogramSnapshots on my local build. I added the line
> export MOZ_TELEMETRY_REPORTING=1
to my mozconfig file though, and that seemed to make it show up. Not sure what that means for testing though (do we need to make sure telemetry is enabled before running the tests?). 

Also, I used Telemetry instead of Services.telemetry in the JavaScript test (it seems to give the same results). Let me know if this isn't correct, though.

Also, in the case that getting the histogram ID fails, we just continue in the loop. I'm not sure if we should be returning the nsresult (rv) or doing something else instead though. 

Let me know if there are any other problems or things to change. Thanks.
Flags: needinfo?(gfritzsche)
Comment on attachment 8840165 [details] [diff] [review]
Initial version of patch.

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

Thanks Gregory, this looks good already and only needs small changes.
Sorry it took a while to review, somehow it dropped off my radar.

I think this should not fail in any test builds, a lot of other test code depends on the snapshots to work.
We can make sure of that by pushing it to our "try" test servers before landing the patch.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +2340,5 @@
> +    mozilla::Telemetry::ID id;
> +    nsresult rv = internal_GetHistogramEnumId(h->histogram_name().c_str(), &id);
> +    if (NS_FAILED(rv)) {
> +      continue;
> +    } else if (gHistograms[id].keyed) {

Lets make this a bit shorter and log warnings too:
if (NS_WARN_IF(NS_FAILED(rv)) || gHistograms[id].keyed) {
  ...

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ +757,5 @@
>      "Keyed histogram add should not record when recording is disabled");
>  });
>  
> +add_task(function* test_histogram_snapshots() {
> +    let keyed = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT");

The indentation should be 2 spaces.

@@ +760,5 @@
> +add_task(function* test_histogram_snapshots() {
> +    let keyed = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT");
> +    keyed.add("a", 1);
> +
> +    //Check that keyed hisograms are not returned

Nit: Space before "Check"
Attachment #8840165 - Flags: feedback+
Assignee

Comment 23

2 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> Thanks Gregory, this looks good already and only needs small changes.
> Sorry it took a while to review, somehow it dropped off my radar.
No problem. I updated the patch with the changes you mentioned. Let me know if there are more problems or things to change. Thanks.

Also, when I ran the other Telemetry tests on my local build (the tests in toolkit/components/telemetry/tests/unit/), one of them kept timing out. Not sure why it was doing that, but hopefully it will work on the try server.
Attachment #8840165 - Attachment is obsolete: true
Attachment #8844681 - Flags: review?(gfritzsche)
Assignee: nobody → olucafont6
Flags: needinfo?(gfritzsche)
Comment on attachment 8844681 [details] [diff] [review]
Made some small changes and fixed some formatting errors.

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

This looks good, thanks.
There is one small typo below.
Also, lets shorten the commit message a little to focus on the results?
E.g.: "Bug xxx - nsITelemetry.histogramSnapshots will no longer return keyed histograms. r=gfritzsche"
The implementation details can be seen from the code changes here.

The timeout was probably with test_ThreadHangStats.js? That one is problematic and needs work, it takes a long time to run.

I pushed the changes to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43bfbe44d997b89bca71ec7435eac4ebc1a9893a

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ +760,5 @@
> +add_task(function* test_histogramSnapshots() {
> +  let keyed = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT");
> +  keyed.add("a", 1);
> +
> +  // Check that keyed hisograms are not returned

Typo, i missed that before: "keyed histograms"
Attachment #8844681 - Flags: review?(gfritzsche) → review+
Priority: -- → P3
Assignee

Comment 25

2 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> Typo, i missed that before: "keyed histograms"
Oh whoops, I missed that as well. Thanks for catching that.

> The timeout was probably with test_ThreadHangStats.js? That one is
> problematic and needs work, it takes a long time to run.
I tried it again and (that time at least) it was test_TelemetrySession.js. Not sure why it's timing out...

I updated the patch. Thanks for all of the help with this bug, I appreciate it.
Attachment #8844681 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8845187 - Flags: review?(gfritzsche)
Comment on attachment 8845187 [details] [diff] [review]
Fixed a spelling error and shortened the commit message.

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

This looks good, thanks!
Attachment #8845187 - Flags: review?(gfritzsche) → review+

Comment 27

2 years ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5733e0b0e096
nsITelemetry.histogramSnapshots will no longer return keyed histograms. r=gfritzsche
Keywords: checkin-needed

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5733e0b0e096
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
backed this out from central, iris told me this made https://treeherder.mozilla.org/logviewer.html#?job_id=85239109&repo=mozilla-inbound failing more frequently
Flags: needinfo?(olucafont6)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 30

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/ca4ae502156e
Backed out changeset 5733e0b0e096 for causing frequent timeouts in test_XHR_timeout.html
Assignee

Comment 31

2 years ago
Alright, I tried adding some debugging output on my local build to find what was causing the timeout. I never actually got the unexpected test failure on my local build, but I have some ideas of what might be causing it. 

When I run the test:
> ./mach mochitest dom/xhr/tests/test_XHR_timeout.html
on my local build, there are a lot of times when the call:
> nsresult rv = internal_GetHistogramEnumId(h->histogram_name().c_str(), &id);
fails (this can also be seen in the test run in comment 29). I outputted the name of each histogram (e.g., h->histogram_name()) that this failed for and attached it as a text file. There are a lot of failures (almost 4000), so I'm wondering if this could be part of the problem.

The other thing I noticed is that we call internal_ShouldReflectHistogram() right before we call internal_GetHistogramEnumId():
https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/toolkit/components/telemetry/TelemetryHistogram.cpp#2277
This function also calls internal_GetHistogramEnumId() to get the id from the histogram name, which is inefficient. I don't know how long each of these calls takes, but maybe calling that twice could somehow contribute to the problem? I'm not really sure about this though.
Flags: needinfo?(olucafont6)
Good details, that helps!
I think this actually uncovered a bug with this approach.

We shouldn't see this many failures. So, looking through the output, there are many examples like:
> sub#XMLHTTPREQUEST_ASYNC_OR_SYNC#content
> CONTENT_LARGE_PAINT_PHASE_WEIGHT#dl

Actually, all histogram names in that output have a "#".
This is the problem:
gHistogramMap is used to map the histogram names to ids.
When we populate gHistogramMap in TelemetryHistogram::InitializeGlobalState(), we use the plain histogram names as they are given in Histograms.json.
For each histogram in Histograms.json, we internally store multiple histograms, using different names to tell them apart.
So e.g. for a keyed histogram "TEST" we can end up having these storage histograms (yes, its convoluted):
TEST#someKey
sub#TEST#someKey
TEST#someKey#content
sub#TEST#someKey#content

So, we could make internal_GetHistogramMapEntry() strip the name accordingly...
But actually the snapshot code is pretty old and i think we could do this much cleaner instead, using something like this instead of the current loop:

> for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) {
>   if (gHistograms[i].keyed) {
>     continue;
>   }
>   Histogram* h = nullptr;
>   internal_GetHistogramByEnumId(i, &h);
Assignee

Comment 33

2 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #32)
> So, we could make internal_GetHistogramMapEntry() strip the name
> accordingly...
> But actually the snapshot code is pretty old and i think we could do this
> much cleaner instead, using something like this instead of the current loop:
> 
> > for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) {
> >   if (gHistograms[i].keyed) {
> >     continue;
> >   }
> >   Histogram* h = nullptr;
> >   internal_GetHistogramByEnumId(i, &h);

Yeah, that does seem better. I tried this and the tests give much less errors now. We do something very similar in the previous for loop:
https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/toolkit/components/telemetry/TelemetryHistogram.cpp#2239-2261
What should we be passing as the third argument (aProcessType) to GetHistogramByEnumId()? Or should we try each different value (GeckoProcessType_Default, GeckoProcessType_Content, and GeckoProcessType_GPU), perhaps in a nested for loop?

Also, in the original loop we were looping over the contents of hs (a StatisticsRecorder::Histograms), but now we would just be looping over gHistograms. Just want to make sure those would be equivalent. Thanks.
Flags: needinfo?(gfritzsche)
I don't think i can help out today before i'm going into vacation.
Chris, you worked on adding child process support here - could you help out?
Flags: needinfo?(gfritzsche)
Flags: needinfo?(chutten)
*lotsa reading*

Looping over `gHistograms` is different since gHistograms has only one entry per histogram id. So TEST will return one Histogram*, not give you TEST#content and sub#TEST#content and so forth.

"Looping" over each process type for GetHistogramByEnumId is perfectly valid (it's how that earlier for loop works, after all), so I think that it's a good thing to prototype and test.

I look forward to working with you on this!
Mentor: chutten
Flags: needinfo?(chutten)
Assignee

Comment 36

2 years ago
(In reply to Chris H-C :chutten from comment #35)
> Looping over `gHistograms` is different since gHistograms has only one entry
> per histogram id. So TEST will return one Histogram*, not give you
> TEST#content and sub#TEST#content and so forth.
> 
> "Looping" over each process type for GetHistogramByEnumId is perfectly valid
> (it's how that earlier for loop works, after all), so I think that it's a
> good thing to prototype and test.

Okay, great, thanks. I tried this and it seems to work. I had it output the name of each histogram though, and it gave different results than the original loop (when running test_XHR_timeout.html at least). So I'm not entirely sure it's iterating over exactly the same histograms. It does seem to still pass the tests in toolkit/components/telemetry/tests/unit/ though. Thanks.
Attachment #8845187 - Attachment is obsolete: true
Comment on attachment 8851151 [details] [diff] [review]
Changed the way CreateHistogramSnapshots() iterates over histograms.

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

Did you forget to put in a r? for review? Did you want me to pass this to try?

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +2273,5 @@
>  
>    // OK, now we can actually reflect things.
>    JS::Rooted<JSObject*> hobj(cx);
> +  for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) {
> +    GeckoProcessType const processTypes[] = { GeckoProcessType_Default, GeckoProcessType_Content, GeckoProcessType_GPU };

Loop invariants should be kept outside of the loop. (Compiler doesn't care, it'll do the right thing (probably), but it makes it easier to comprehend when reading (at least for me))

@@ +2274,5 @@
>    // OK, now we can actually reflect things.
>    JS::Rooted<JSObject*> hobj(cx);
> +  for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) {
> +    GeckoProcessType const processTypes[] = { GeckoProcessType_Default, GeckoProcessType_Content, GeckoProcessType_GPU };
> +    size_t numTypes = (includeGPUProcess ? 3 : 2);

I guess this should probably be numProcessTypes, but that's getting pedantic even for me.

@@ +2276,5 @@
> +  for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) {
> +    GeckoProcessType const processTypes[] = { GeckoProcessType_Default, GeckoProcessType_Content, GeckoProcessType_GPU };
> +    size_t numTypes = (includeGPUProcess ? 3 : 2);
> +    for (size_t type = 0; type < numTypes; ++type) {
> +      if (gHistograms[i].keyed) {

Can move this to the outer loop, as with everything down to the internal_GetHistogramByEnumId call

@@ +2292,5 @@
> +
> +      Histogram* original = h;
> +#if !defined(MOZ_WIDGET_ANDROID)
> +      if (subsession) {
> +        h = internal_GetSubsessionHistogram(*h);

Thanks to bug 1350765, you'll likely need to rebase before landing. On doing so, use the version of internal_GetSubsessionHistogram that also takes an id: it's faster.
Assignee

Comment 38

2 years ago
(In reply to Chris H-C :chutten from comment #37)
> Did you forget to put in a r? for review? Did you want me to pass this to
> try?

I actually wasn't really sure if the patch was finished yet, so wasn't sure what to ask. I was thinking about testing more to see if we were iterating over the same histograms as before. I thought I would post what I had so far though. I didn't find where StatisticsRecorder::histograms_ is populated, but I think we are probably still doing the same thing. 

Thanks for all of the suggestions, I updated the patch. I think we can probably push it to the try server now.

> Thanks to bug 1350765, you'll likely need to rebase before landing. On doing
> so, use the version of internal_GetSubsessionHistogram that also takes an
> id: it's faster.

Okay, sure. When the patch for that bug lands I'll update it. I wonder if adding a version of internal_ShouldReflectHistogram that also takes an ID would be a good idea, since we call it in the loop when we already have the ID. Thanks.
Attachment #8851151 - Attachment is obsolete: true
Attachment #8851764 - Flags: review?(chutten)
Comment on attachment 8851764 [details] [diff] [review]
Implemented the changes described in comment 37.

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

(In reply to Gregory Moore [:gmoore] from comment #38)
> Okay, sure. When the patch for that bug lands I'll update it. I wonder if
> adding a version of internal_ShouldReflectHistogram that also takes an ID
> would be a good idea, since we call it in the loop when we already have the
> ID. Thanks.

Sounds like an excellent plan for a follow-up bug.

--
The patch looks good, I've pushed it to try over in this direction: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f2598edec1e680941eda942a1dbed15c421611f

(though I accidentally pushed it atop inbound instead of central, so hopefully we won't see too much in the way of noise)
Attachment #8851764 - Flags: review?(chutten) → review+
try is green and bug 1350765 is stalled, so you get to win the checkin race :)

(sorry for the delay, was traveling for work)
Keywords: checkin-needed

Comment 41

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d5e6b3055d
nsITelemetry.histogramSnapshots will no longer return keyed histograms. r=chutten
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c6d5e6b3055d
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.