Closed Bug 918557 Opened 6 years ago Closed 2 years ago

Is TelemetryPing's histogram's cache necessary?

Categories

(Toolkit :: Telemetry, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: njn, Assigned: diorahman, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=js][good-next-bug])

Attachments

(1 file, 1 obsolete file)

TelemetryPing caches histograms in a property called |_histograms|.  If Telemetry.getHistogramById is slow, then it might be worthwhile.  Otherwise, it could be removed.
Let's set this up as a mentored bug.
Blocks: 1201022
Flags: needinfo?(alessio.placitelli)
Priority: -- → P4
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
Mentor: alessio.placitelli
Flags: needinfo?(alessio.placitelli)
Hey, would you be interested in working on this?
Flags: needinfo?(rohan17089)
Yes i am.
Flags: needinfo?(rohan17089)
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
Flags: needinfo?(alessio.placitelli)
(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.
Assignee: nobody → rohan17089
Flags: needinfo?(alessio.placitelli)
Whiteboard: [lang=js][good-next-bug]
Clearing the assignee for inactivity :)
Assignee: rohan17089 → nobody
Mentor: chutten
Assignee: nobody → diorahman
Status: NEW → ASSIGNED
Attachment #8955744 - Flags: review?(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?
Attachment #8955744 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f26ab4fb6ab1
Remove TelemetryPing's histogram cache. r=chutten
Keywords: checkin-needed
Attachment #8955744 - Attachment is obsolete: true
Flags: needinfo?(diorahman)
Comment on attachment 8956112 [details] [diff] [review]
Remove TelemetryPing's histogram cache

Fix eslint spurious message
Attachment #8956112 - Flags: review?(chutten)
(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 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?
Attachment #8956112 - Flags: review?(chutten) → review+
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)
Let's try this one again, shall we?
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fec9d07e4e6
Remove TelemetryPing's histogram cache. r=chutten
Keywords: checkin-needed
(In reply to Chris H-C :chutten from comment #17)
> Let's try this one again, shall we?

:pray:
https://hg.mozilla.org/mozilla-central/rev/0fec9d07e4e6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.