1162042 - Add a slightly easier API for adding simple measures.

Status: RESOLVED WONTFIX

(Toolkit :: Telemetry, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

This builds on bug 940807 by letting people send data blobs into the simpleMeasures, and not rely on leaving a callback sitting around.

(I'm writing this because I ran into a problem trying to add UITelemetry data from an add-on, but the addon gets shutdown before the simpleMeasures were collected, so nothing showed up even though it appeared in about:telemetry.)

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first cut…

I'm not sure where to add tests for this, but will happily do so if someone can point me at the right place.  :)

Comment 2

[Chenxia Liu [:liuche]]

Comment on attachment 8602116 [details] [diff] [review]
The first cut…

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

Looks like tests could be added to toolkit/components/telemetry/tests/unit/test_TelemetrySession.js, though it looks like the test explicitly checks for not including UITelemetry...I'm not familiar with what extended payload fields are, and when we should turn them on/test for them.

Looks good to me, just the one question. r+ if that's fine, just send it back to me.

::: toolkit/components/telemetry/UITelemetry.jsm
@@ +78,5 @@
>    },
>  
>    /**
> +   * Holds the data that provides UITelemetry's simple
> +   * measurements. That data is mapped to unique names,

Perhaps add a mention of why one would use this as opposed to addSimpleMeasuresFunctions, e.g., in the case of addons leaving dead callbacks. I would assume that we prefer using simpleMeasuresFunction? and if so, the comment should state that.

@@ +83,5 @@
> +   * and should be registered with addSimpleMeasure.  Note: it
> +   * will be passed to a simpleMeasureFunction with the same
> +   * name, if one exists.
> +   */
> +  _simpleMeasures: {},

_simpleMeasuresData to be more clear.

@@ +196,2 @@
>      for (let name in this._simpleMeasureFunctions) {
> +      result[name] = this._simpleMeasureFunctions[name](result[name]);

If you've registered a function and also added simple measures data, and the addon that registered the callback functions gets shut down, what does the callback return, undefined or just die? Will that overwrite what you passed in as simpleMeasuresData?

@@ +200,5 @@
>    },
>  
>    /**
> +   * Allows the caller to add a simple measure. aName is a unique
> +   * identifier used as they key for the simple measurement in the

typo: "the key"

@@ +203,5 @@
> +   * Allows the caller to add a simple measure. aName is a unique
> +   * identifier used as they key for the simple measurement in the
> +   * object that getSimpleMeasures returns.
> +   */
> +  addSimpleMeasure: function(aName, aData) {

addSimpleMeasureData

@@ +226,3 @@
>     * Allows the caller to register functions that will get called
>     * for simple measures during a Telemetry ping. aName is a unique
>     * identifier used as they key for the simple measurement in the

...I guess you copied it from here, so you could update this too.

Comment 3

[Chenxia Liu [:liuche]]

> Looks like tests could be added to
> toolkit/components/telemetry/tests/unit/test_TelemetrySession.js, though it
> looks like the test explicitly checks for not including UITelemetry...I'm
> not familiar with what extended payload fields are, and when we should turn
> them on/test for them.

mconley, is that something we'd want to do, enable testing of UITelemetry?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

I do like tests and testing. I would happily review tests for UITelemetry.jsm.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

So, I don't think we want to do this this way anymore, cause there are better ways to record things, and we're kinda getting rid of simple measures.  Resolving wontfix, assuming that's okay with the rest of you.  :)