1221619 - Add telemetry to memory tool

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P3)

Description

[Jordan Santell [:jsantell] [@jsantell]]

We have some telemetry for snapshots, but nothing for the client. We should add some things like, which breakdown presets are people using, are people inverting often, recording stacks, etc

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add telemetry to memory tool

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Not sure how to test telemetry data -- verified that the telemetry methods were being called via console.logs, but didn't see any of these show up in about:telemetry...

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #2)
> Not sure how to test telemetry data -- verified that the telemetry methods
> were being called via console.logs, but didn't see any of these show up in
> about:telemetry...

Is toolkit.telemetry.enabled true in your development profile?  If not, flip it and restart.  Data should then appear in about:telemetry.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8711910 [details] [diff] [review]
Add telemetry to memory tool

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

::: devtools/client/memory/telemetry.js
@@ +7,5 @@
> +// NB: Ensure that *every* exported function is wrapped in `makeInfallible` so
> +// that our probes don't accidentally break code that actually does productive
> +// work for the user!
> +
> +const { telemetry } = require("Services");

nit: investigate if it'd make more sense to use the devtools wrapper around telemetry (devtools/client/shared/telemetry.js)

@@ +27,5 @@
> +  histogram.add(1);
> +}, "devtools/client/memory/telemetry#countExportSnapshot");
> +
> +/**
> + * @param {Boolean} inverted    True if the census was inverted, false

Is there a reason these param comments are diff then the other ones in this tool? Generally the param description is on the next line, idented to the {Type}, ya?

/**
 * @param {Boolean} inverted
 *        True if the census was inverted, false
 */

@@ +41,5 @@
> +exports.countCensus = makeInfallible(function ({ inverted, filter, diffing, breakdown }) {
> +  let histogram = telemetry.getHistogramById("DEVTOOLS_MEMORY_INVERTED_CENSUS");
> +  histogram.add(!!inverted);
> +
> +  histogram = telemetry.getHistogramById("DEVTOOLS_MEMORY_FILTER_CENSUS");

Wouldn't DEVTOOLS_MEMORY_FILTER_CENSUS and DEVTOOLS_MEMORY_INVERTED_CENSUS both get recorded for a DIFF rather than just CENSUS? Filter isn't even implemented for diffing right now; we'd have no way to know if people are always inverting censues, but leaving diffs uninverted, for example -- not sure best way to handle that, or if it's a valid concern though

@@ +49,5 @@
> +  histogram.add(!!diffing);
> +
> +  histogram = telemetry.getKeyedHistogramById("DEVTOOLS_MEMORY_BREAKDOWN_CENSUS_COUNT");
> +  if (breakdown === breakdowns.coarseType.breakdown) {
> +    histogram.add("Coarse Type");

use const/enums for these histos

@@ +57,5 @@
> +    histogram.add("Object Class");
> +  } else if (breakdown === breakdowns.internalType.breakdown) {
> +    histogram.add("Internal Type");
> +  } else {
> +    histogram.add("Other: " + JSON.stringify(breakdown));

This will be hard to get value out of it -- maybe just "Custom" here? If we want to track what kind of custom breakdowns people are using (unlikely many at all for this undocumented feature), maybe we can split it up into "features" for the breakdown? Not sure.

@@ +78,5 @@
> +  histogram.add(1);
> +
> +  histogram = telemetry.getKeyedHistogramById("DEVTOOLS_MEMORY_BREAKDOWN_DOMINATOR_TREE_COUNT");
> +  if (breakdown === dominatorTreeBreakdowns.coarseType.breakdown) {
> +    histogram.add("Coarse Type");

use const/enums for these histos

::: toolkit/components/telemetry/Histograms.json
@@ +9256,5 @@
>      "low": "20000000",
>      "high": "40000000",
>      "n_buckets": 20
>    },
> +  "DEVTOOLS_MEMORY_TAKE_SNAPSHOT_COUNT": {

These should be with the other DEVTOOLS_ histograms

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #2)
> > Not sure how to test telemetry data -- verified that the telemetry methods
> > were being called via console.logs, but didn't see any of these show up in
> > about:telemetry...
> 
> Is toolkit.telemetry.enabled true in your development profile?  If not, flip
> it and restart.  Data should then appear in about:telemetry.

Ah, thanks! I didn't have it set for whatever reason, but now that it is set I can see the data for my new probes.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #4)
> Comment on attachment 8711910 [details] [diff] [review]
> Add telemetry to memory tool
> 
> Review of attachment 8711910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/memory/telemetry.js
> @@ +7,5 @@
> > +// NB: Ensure that *every* exported function is wrapped in `makeInfallible` so
> > +// that our probes don't accidentally break code that actually does productive
> > +// work for the user!
> > +
> > +const { telemetry } = require("Services");
> 
> nit: investigate if it'd make more sense to use the devtools wrapper around
> telemetry (devtools/client/shared/telemetry.js)

This seems to facilitate timing how long people are using a given tool, which is disjoint from the set of things I added probes for in this patch. I can add this measurement in another patch.

> @@ +41,5 @@
> > +exports.countCensus = makeInfallible(function ({ inverted, filter, diffing, breakdown }) {
> > +  let histogram = telemetry.getHistogramById("DEVTOOLS_MEMORY_INVERTED_CENSUS");
> > +  histogram.add(!!inverted);
> > +
> > +  histogram = telemetry.getHistogramById("DEVTOOLS_MEMORY_FILTER_CENSUS");
> 
> Wouldn't DEVTOOLS_MEMORY_FILTER_CENSUS and DEVTOOLS_MEMORY_INVERTED_CENSUS
> both get recorded for a DIFF rather than just CENSUS?

Yes, that was intentional.

> Filter isn't even
> implemented for diffing right now;

Filtering is implemented for diffing.

> we'd have no way to know if people are
> always inverting censues, but leaving diffs uninverted, for example -- not
> sure best way to handle that, or if it's a valid concern though

This is true, but I'm not sure that it matters a ton.

> @@ +57,5 @@
> > +    histogram.add("Object Class");
> > +  } else if (breakdown === breakdowns.internalType.breakdown) {
> > +    histogram.add("Internal Type");
> > +  } else {
> > +    histogram.add("Other: " + JSON.stringify(breakdown));
> 
> This will be hard to get value out of it -- maybe just "Custom" here? If we
> want to track what kind of custom breakdowns people are using (unlikely many
> at all for this undocumented feature), maybe we can split it up into
> "features" for the breakdown? Not sure.

Yeah, I was on the fence too. I guess it will be interesting just to see if anyone uses custom breakdowns at all (how would they even know they can?)

> > +  "DEVTOOLS_MEMORY_TAKE_SNAPSHOT_COUNT": {
> 
> These should be with the other DEVTOOLS_ histograms

They are with the other DEVTOOLS_* histograms, at the bottom of them.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add telemetry to memory tool

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #6)
> (In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from
> > nit: investigate if it'd make more sense to use the devtools wrapper around
> > telemetry (devtools/client/shared/telemetry.js)
> 
> This seems to facilitate timing how long people are using a given tool,
> which is disjoint from the set of things I added probes for in this patch. I
> can add this measurement in another patch.

We already seem to have this in place now for the memory tool, so I don't think another patch is needed.

Comment 9

[Benjamin Smedberg]

Comment on attachment 8712401 [details] [diff] [review]
Add telemetry to memory tool

data-review=me

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add telemetry to memory tool

Change commit message to reflect r=jsantell and data-review=bsmedberg

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab60d707bf07

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2795ef07d96b

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2795ef07d96b