918557 - Is TelemetryPing's histogram's cache necessary?

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Nicholas Nethercote [inactive]]

TelemetryPing caches histograms in a property called |_histograms|.  If Telemetry.getHistogramById is slow, then it might be worthwhile.  Otherwise, it could be removed.

Comment 1

[Alessio Placitelli [:Dexter]]

Let's set this up as a mentored bug.

Comment 2

[Alessio Placitelli [:Dexter]]

We should change TelemetrySession.jsm [1]:

- remove the |_histograms| variable;
- change handleMemoryReport to call Telemetry.getHistogramById(...)/getKeyedHistogramById(...) without caching in |_histograms|;
- call |.add()|

We can test that this works correctly by running the telemetry tests:

> ./mach test toolkit/components/telemetry/tests/unit


[1] - https://searchfox.org/mozilla-central/rev/b24e6342d744c5a83fab5c15972e11eeb69d68e6/toolkit/components/telemetry/TelemetrySession.jsm#1201-1215

Comment 3

[Alessio Placitelli [:Dexter]]

Hey, would you be interested in working on this?

Comment 4

[Rohan Rajpal]

Yes i am.

Comment 5

[Rohan Rajpal]

Hey,i did the necessary changes and made the build.

The test gives these results 
https://drive.google.com/file/d/1nMGguPCtqtqSi9opoWQSJ2lV-4pCvfZE/view?usp=sharing

Just one thing though,how do you sync your local repositories with the online mozilla ones.It has been a while and i have to update it.Hg update didnt work for me(got to know about hg update from irc)

Hg update gives me this
rohan@rohan:~/src/mozilla-central$ hg update
0 files updated, 0 files merged, 0 files removed, 0 files unresolved

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Rohan Rajpal from comment #5)
> Hey,i did the necessary changes and made the build.
> 
> The test gives these results 
> https://drive.google.com/file/d/1nMGguPCtqtqSi9opoWQSJ2lV-4pCvfZE/
> view?usp=sharing

The test is failing. Do you need any help in figuring out why this is happening?

> Just one thing though,how do you sync your local repositories with the
> online mozilla ones.It has been a while and i have to update it.Hg update
> didnt work for me(got to know about hg update from irc)

You might want to read the documentation about our workflow and mercurial here: http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/unifiedrepo.html#working-with-the-unified-repo-vanilla-mercurial

To fetch the latest update from the mozilla-central repo, you should pull: "hg pull". Then you should rebase your changeset toward the newest tip.

Comment 7

[Alessio Placitelli [:Dexter]]

Clearing the assignee for inactivity :)

Comment 8

[Dhi Aurrahman]

Attachment: Remove TelemetryPing's histogram cache

Comment 9

[Chris H-C :chutten]

Comment on attachment 8955744 [details] [diff] [review]
Remove TelemetryPing's histogram cache

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

This looks good, thank you for your contribution! I'll mark this for checkin.

Have you considered what you might want to work on next?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f26ab4fb6ab1
Remove TelemetryPing's histogram cache. r=chutten

Comment 11

[Eliza Balazs [:ebalazs_]]

Backed out changeset f26ab4fb6ab1 (bug 918557) fo ES lint failure in /builds/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySession.jsm:1191:7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f26ab4fb6ab185c7223eabe96606a51c33ec8453&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=success&filter-classifiedState=unclassified&selectedJob=165999507

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=165999507&repo=mozilla-inbound&lineNumber=65

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e7f2f3b0c920a16f2b312cf92385976d30d1e74f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=success

Comment 12

[Dhi Aurrahman]

Attachment: Remove TelemetryPing's histogram cache

Comment 13

[Dhi Aurrahman]

Comment on attachment 8956112 [details] [diff] [review]
Remove TelemetryPing's histogram cache

Fix eslint spurious message

Comment 14

[Dhi Aurrahman]

(In reply to Eliza Balazs [:ebalazs_] from comment #11)
> Backed out changeset f26ab4fb6ab1 (bug 918557) fo ES lint failure in
> /builds/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySession.
> jsm:1191:7
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=f26ab4fb6ab185c7223eabe96606a51c33ec8453&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=runnable&filter-
> resultStatus=running&filter-resultStatus=pending&filter-
> resultStatus=success&filter-
> classifiedState=unclassified&selectedJob=165999507
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=165999507&repo=mozilla-
> inbound&lineNumber=65
> 
> Backout:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=e7f2f3b0c920a16f2b312cf92385976d30d1e74f&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=runnable&filter-
> resultStatus=running&filter-resultStatus=pending&filter-resultStatus=success

I have another patch in review: Attachment #8956112 [details] [diff]

Sorry for making eslint angry :(

Comment 15

[Chris H-C :chutten]

Comment on attachment 8956112 [details] [diff] [review]
Remove TelemetryPing's histogram cache

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

I'm sorry I didn't catch this in review!

Did you run `mach eslint` on your machine to make sure there aren't any other subtle failures lurking in this patch?

Comment 16

[Dhi Aurrahman]

Yes, I did for the last patch. I believe I ran it before, but yeah, could be slipped.

$ ./mach lint toolkit/components/telemetry/TelemetrySession.jsm
✖ 0 problems (0 errors, 0 warnings)

Comment 17

[Chris H-C :chutten]

Let's try this one again, shall we?

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fec9d07e4e6
Remove TelemetryPing's histogram cache. r=chutten

Comment 19

[Dhi Aurrahman]

(In reply to Chris H-C :chutten from comment #17)
> Let's try this one again, shall we?

:pray:

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/0fec9d07e4e6