659196 - Improve telemetry metadata, introduce simple counters

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[(dormant account)]

Attachment: wip

This patch removes XPCOM abi, adds, arch, OSversion, cpucount, device, manufacturer, hardware to telemetry metadata. It also sicks uptime and startup-info into the new .simpleCounters dictionary.

Comment 1

[(dormant account)]

Attachment: more informative telemetry ping

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 534932 [details] [diff] [review]
more informative telemetry ping

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

Probably should have spotted this in the first patch, but perhaps we need versioning in the telemetry URL or something to handle schema changes

::: toolkit/components/telemetry/TelemetryPing.js
@@ +123,2 @@
>      ID: ai.ID,
>      version: ai.version,

Could change this to appVersion to avoid the conflict below, unless you're concerned about breaking comparisons with earlier versions

@@ +145,5 @@
> +      break;
> +    case "memsize":
> +      // Send RAM size in megabytes. Rounding because sysinfo doesn't
> +      // always provide RAM in multiples of 1024.
> +      value = Math.round(value/1024/1024)

Nit: spaces around operators

Comment 3

[Mike Hommey [:glandium]]

Attachment: Improve telemetry metadata, introduce simple counters

I renamed the version variable as suggested and also renamed some other variables for some kind of consistency. This makes the sysInfo loop simpler (but the diff less readable)

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 535327 [details] [diff] [review]
Improve telemetry metadata, introduce simple counters

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

I was somewhat lax in my original review of this code in not requiring better comments for the methods so I'd like to see these before giving this r+, otherwise it all looks fine.

::: toolkit/components/telemetry/TelemetryPing.js
@@ +115,5 @@
>  }
>  
>  /**
> + * @return request metadata. This stuff remains static across multiple
> + * pings, sort of like http headers.

Thinking about it this probably needs a better comment. Something like:

Gets metadata about the platform the application is running on. This should remain consistent across multiple telemetry pings.

@param  reason
        The reason for the telemetry ping, this will be included in the returned metadata,
@return The metadata as a JS object

Also the norm is to indent the second line to line up with the start of the first.

@@ +150,5 @@
> +  return ret;
> +}
> +
> +/**
> + * @return simple measurements (counters). This returns a dictionary with js primitives as values.

Needs a better comment, as above.

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +86,5 @@
>    let tc = payload.histograms[TELEMETRY_SUCCESS]
>    do_check_eq(uneval(tc), 
>                uneval(expected_tc));
> +
> +  return ret;

What is this for?

Comment 5

[Mike Hommey [:glandium]]

(In reply to comment #4)
> ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
> @@ +86,5 @@
> >    let tc = payload.histograms[TELEMETRY_SUCCESS]
> >    do_check_eq(uneval(tc), 
> >                uneval(expected_tc));
> > +
> > +  return ret;
> 
> What is this for?

Nothing, obviously.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Improve telemetry metadata, introduce simple counters

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 536482 [details] [diff] [review]
Improve telemetry metadata, introduce simple counters

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

Thanks

::: toolkit/components/telemetry/TelemetryPing.js
@@ +115,5 @@
>  }
>  
>  /**
> + * Gets metadata about the platform the application is running on. This
> + * should remain consistent across multiple telemetry pings.

Nit, we normally put a blank line between the comment and the @param parts.

@@ +155,5 @@
> +}
> +
> +/**
> + * Gets a series of simple measurements (counters). At the moment, this
> + * only returns startup data from nsIAppStartup.getStartupInfo().

Same here.

Comment 8

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/998d4edbf4b3