Closed Bug 1280234 Opened 8 years ago Closed 6 years ago

Allow Telemetry to be sent to Mozilla from a WebExtension

Categories

(WebExtensions :: General, defect, P2)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: andy+bugzilla, Assigned: jhirsch, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [telemetry] triaged)

Attachments

(1 file, 7 obsolete files)

Bug 1280234 - Expose Telemetry APIs to trusted WebExtensions;
59 bytes, text/x-review-board-request
aswan
: review+
Details 
It would be nice to allow telemetry to be sent to Mozilla servers, to allow add-ons like the TestPilot to send telemetry data. There's a few telemetry APIs, maybe just starting with a ping.

This API would need to be locked down to Mozilla only users since this is specific to Mozilla.
Depends on: 1280235
Priority: -- → P2
Whiteboard: [telemetry] triaged
Other extensions already use Google Analytics. Why not TestPilot?
(In reply to Andy McKay [:andym] from comment #0)
> It would be nice to allow telemetry to be sent to Mozilla servers, to allow
> add-ons like the TestPilot to send telemetry data. There's a few telemetry
> APIs, maybe just starting with a ping.
> 
> This API would need to be locked down to Mozilla only users since this is
> specific to Mozilla.

This is also something that would be helpful for user onboarding experiments. Google Analytics can be used via the measurement protocol for basic analytics, but telemetry is needed for retention since retention is calculated by finding the difference between profile creation date and now.
Test Pilot currently uses Telemetry and Google Analytics for capturing data (depending on the experiment).  We expect to continue to use both pipelines as we answer different questions with the two (I expand a bit on this in some documentation I'm writing: https://github.com/mozilla/testpilot/pull/1662/files )

It's likely that converting to a web extension is going to be a 2017 goal for the Test Pilot team, so I'd like to reopen this discussion and talk about options.  Thanks!
From bug 1322507, groovecoder is already working on an experiment:

https://github.com/groovecoder/web-ext-telemetry-api-experiment
Hi Andy, what would be needed to implement this API? Is someone from your team available to mentor, if someone from the Test Pilot team had bandwidth to write a patch?
Flags: needinfo?(amckay)
Bob is just learning about telemetry and would be happy to mentor this I'm sure. With bug 1280235 landing in the next few iterations, this becomes doable. Basically, at this point its figure out what the API should be (maybe something like groovecoder proposed in comment 5?) and then land it into the WebExtensions tree.
Mentor: bob.silverberg
Flags: needinfo?(amckay)
There are two paths to take here:

(1) Expose our Telemetry APIs to the WebExtension.
That would forward our histogram, scalar, event & custom ping APIs to the extension.
Recording existing histograms, scalars & events works right now.
Recording new ones from add-ons is on the roadmap, starting with events in Q2.

(2) Build a higher-level abstraction over Telemetry for WebExtensions.

Jared, what is the specific motivation from test pilot?
Maybe we should start with solving that?
Flags: needinfo?(jhirsch)
Not speaking for Jared, but our specific motivation in the Blok/Tracking Protection and Containers experiments were simply to send data to Mozilla Telemetry pipeline with a 100% WebExtension add-on. With Blok we were only able to do it by opening a BroadcastChannel to the (SDK) Test Pilot add-on.

So when SDK support goes away, that trick won't work anymore.

Similarly, shield add-ons will need to send Telemetry, and when they have to be 100% WebExtension add-ons, we will need a WebExtension API to submit data to Mozilla Telemetry.

So, (1) is the top priority for individual experiments. And (2) could be a follow-up.

I'll let Jared speak to the new Test Pilot metrics broker library which may need (2) sooner than later.
One thing we'd like in Screenshots is the ability to read the Telemetry preference, so that the extension's own custom analytics could be turned off by that preference.  (We're doing that now with an Embedded Extension and bootstrap.js wrapper: https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.js)
(In reply to Ian Bicking (:ianb) from comment #10)
> One thing we'd like in Screenshots is the ability to read the Telemetry
> preference, so that the extension's own custom analytics could be turned off
> by that preference.  (We're doing that now with an Embedded Extension and
> bootstrap.js wrapper:
> https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.
> js)

That sounds like a good method for a WebExtensions Telemetry API. It should be added to the design of the new API.
Test Pilot currently uses two Telemetry APIs:

* `TelemetryController.submitExternalPing`, to send extra experiment-specific fields to Telemetry[1]
* `TelemetryController.getCurrentPingData`, to get some Telemetry-specific fields that are then forwarded to Ping Centre[2].

If the API would be restricted to Mozilla trusted WebExtensions for 57+, I'd think it would make sense to expose both functions.

I'm happy to write the patch; paging Bob for guidance on first steps.


[1] https://github.com/mozilla/testpilot/blob/master/addon/src/lib/metrics/experiment.js#L58
[2] https://github.com/mozilla/testpilot/blob/master/addon/src/lib/metrics/experiment.js#L64
Flags: needinfo?(jhirsch) → needinfo?(bob.silverberg)
(In reply to Jared Hirsch [:_6a68] (please use 'needinfo' :-) from comment #13)
> Test Pilot currently uses two Telemetry APIs:
> 
> * `TelemetryController.submitExternalPing`, to send extra
> experiment-specific fields to Telemetry[1]
> * `TelemetryController.getCurrentPingData`, to get some Telemetry-specific
> fields that are then forwarded to Ping Centre[2].
> 

I don't understand why this is needed. What exactly is Ping Centre? Is it something that would be a common requirement for internal extensions to work with?

> If the API would be restricted to Mozilla trusted WebExtensions for 57+, I'd
> think it would make sense to expose both functions.
> 
> I'm happy to write the patch; paging Bob for guidance on first steps.
> 

The first step would be to come up with a proposed API, which I believe Georg started at https://docs.google.com/document/d/1eV6FshjdbcTV3N477Da505Uv2aFiMOVOUk9XxLhd1Jo/. We should document what API methods we need, along with their inputs and outputs. At this point it does seem like it makes more sense to go with option 1 from comment 8, as I don't really see a need for a higher level abstraction at this time. The API code will be small enough that it will be easy to refactor later if we need to, and the fact that it's going to be locked down means the number of consumers will be low, and easy to work with if we do choose to change the API at a later date.

We could document all of the APIs we think we might need in the future, but that doesn't mean we necessarily have to implement them all now. I think making an effort to document everything now is still good, as it may help us to see any gaps or issues with the proposed API. It probably makes sense to see what other consumers of a WebExtensions Telemetry API might need.

Once we've got an API design that we're happy with, we can choose which ones to implement. At that point you'd need to create a schema, some code that implements the APIs and some tests. I can help you with all of that when the time comes. This is also somewhat blocked by bug 1280235, but I think it still makes sense to start working on it now, even if it won't be able to land until after 1280235.

Does that give you enough to start with?
Flags: needinfo?(bob.silverberg)
> I don't understand why this is needed. What exactly is Ping Centre? Is it something that would be a common requirement for internal extensions to work with?

Ping Centre is the repurposed Tiles pipeline. Test Pilot has been exploring it as an alternative to the Telemetry pipeline for data processing, so we send similar events to both services. Note that a very small subset of Telemetry fields is sent to Ping Centre, see https://github.com/mozilla/testpilot/blob/master/addon/src/lib/metrics/experiment.js#L70-L77.

> Does that give you enough to start with?

Yes, definitely. I'll add a few questions below for future steps, but for now, I can expand that gdoc to list out current use cases, define API methods and data types, and separately hand wave at potential future use cases. I'll ni? you when that's done.

Some other questions / discussion points:


# Telemetry API access control

I could see us either checking against a hardcoded whitelist, or adding a new 'telemetry' permission to manifest.json.

The permission approach would be better for Test Pilot experiments, since they aren't coordinated with the Firefox release trains. However, in that case, it seems that self-hosted non-Mozilla WebExtensions could presumably access the Telemetry API, and if we include the getCurrentPingData method, evil WebExtensions could use this to fingerprint users.

Maybe we could handle off-cycle Test Pilot experiments by using a whitelist that can be updated by an off-cycle system addon?

Or maybe there's a way to verify that a given addon was signed by the Mozilla internal key?


# Other possible consumers

Aside from Test Pilot experiments, I wonder if any system addons could be reimplemented as WebExtensions with the addition of this API. I'll ping the go-faster list to see if anyone wants to add use cases / comments to this bug.


# Getting broader feedback

What would be a good list to ping for feedback on a draft API? Is there an internal-focused WebExtensions or addons mailing list that could work, or should we just have the discussion in this bug?
Assignee: nobody → jhirsch
(In reply to Jared Hirsch [:_6a68] (please use 'needinfo' :-) from comment #15)
> Or maybe there's a way to verify that a given addon was signed by the
> Mozilla internal key?

Please give it a permission. Recognising that the add-on has the ability to access the permission and has been signed as a Mozilla add-on will be done in bug 1280235.
Perfect! That explains why we needed to block on that bug. Thanks andym
Quick update - It looks like Test Pilot is going to expose a draft Telemetry API to Test Pilot addons via either a WebExtension API Experiment or a bootstrapped wrapper that will be used to embed all Test Pilot addons.

Once we get some experience with that API, we'll propose landing it in Firefox.
I'd just like to give a quick shout to say that lockbox (new mozilla pw manager) is being written as a webextension and may stand to benefit from this if it were to land as a (semi-public) API in firefox. We are already on the Test Pilot train so will be able to use their solution in the short term, but its likely that after that we would be able to make use of a more general solution.
I'd like to get back to this bug soon. I have to land some Telemetry in Screenshots for 58 (bug 1412411), but post-58 it would be nice to try to move the Telemetry code into the webextension part.

Since this bug was last updated, Test Pilot experiments are asked to use GA instead of Telemetry as a general rule. So, while there's less of a need for a Telemetry API for experiments, it still would be nice to be able to use a pure webextension system addon and be able to increment Telemetry scalars.
Sounds good, Jared. Let me know if there's anything I can do to help you move forward with this.
Component: WebExtensions: Untriaged → WebExtensions: General
Flags: needinfo?(amckay)
Blocks: 1419884
Flags: needinfo?(amckay)
Blocks: 1414409
No longer blocks: 1419884
Lockbox would also appreciate any new efforts here. We currently send telemetry as a bootstrapped extension but if this landed it would go a long way towards allowing us to de-bootstrap.
I don't think there's anything blocking this bug other than finding somebody with the time and motivation to do it.  This is of interest to a broad enough set of consumers that I think we should land it directly in central with the mozillaAddons permission required.

If somebody from Test Pilot/Lockbox/Shield/whatever wants to work on it, we now have some docs about how to implement a WebExtensions API at:
https://firefox-source-docs.mozilla.org/toolkit/components/extensions/webextensions/index.html

This would be a good opportunity to have somebody test drive the API development process so we can clarify the docs as needed and address any other rough edges along the way, so that creating other APIs in the future for Test Pilot, Shield, etc. is as smooth as possible.
I am 1000% on board, happy to help.  I claim there are a few different telemetry-useful things, which can be one or several apis:

- READ existing telemetry pings (general, or certain types)
- SEND a telemetry ping
  - plain text
  - using the pioneer style encrypted ping
- Get the CLIENT ID from telemetry
- Permissions:  get a list of the telemetry / extended telemetry / shield / pioneer permissions

Some pieces I personally don't know
- how to test the webext experiment
- having a webExt experiment that exposes 'multiple things'.  Should there be telemetry.send / telemetry.clientId as separate permissions?
- PI / QA for the webext experiment?

Thoughts are very welcome!

GL
(In reply to Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat ) from comment #24)
> I am 1000% on board, happy to help.  I claim there are a few different
> telemetry-useful things, which can be one or several apis:
> 
> - READ existing telemetry pings (general, or certain types)
> - SEND a telemetry ping
>   - plain text
>   - using the pioneer style encrypted ping
> - Get the CLIENT ID from telemetry
> - Permissions:  get a list of the telemetry / extended telemetry / shield /
> pioneer permissions

Good point, I'm guessing that sending a ping is the most widely needed thing, perhaps this should be split into multiple bugs.

> Some pieces I personally don't know
> - how to test the webext experiment
> - having a webExt experiment that exposes 'multiple things'.  Should there
> be telemetry.send / telemetry.clientId as separate permissions?
> - PI / QA for the webext experiment?

I don't think there's any reason to do this as an experiment, as I said in the previous comment I think this should just land in central behind the "mozillaAddons" permission.  With that guard, I think a single additional "telemetry" permission that covers all the requested features would be adequate but more fine-grained permissions are also doable if others feel strongly that they would be useful.  In any case, this makes me think that perhaps we need clearer documentation about experiments and privileged APIs and how they fit together...
Jared is out sick today, but since this is getting some discussion I want to mention that he is planning on doing this bug in Q1.  He'll probably have questions so thanks for helping out when he asks. :)
(In reply to Andrew Swan [:aswan] from comment #25)
> (In reply to Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )
> from comment #24)
> > I am 1000% on board, happy to help.  I claim there are a few different
> > telemetry-useful things, which can be one or several apis:
> > 
> > - READ existing telemetry pings (general, or certain types)
> > - SEND a telemetry ping
> >   - plain text
> >   - using the pioneer style encrypted ping
> > - Get the CLIENT ID from telemetry
> > - Permissions:  get a list of the telemetry / extended telemetry / shield /
> > pioneer permissions
> 
> Good point, I'm guessing that sending a ping is the most widely needed
> thing, perhaps this should be split into multiple bugs.

Sending a plain ping is very valuable, but we can also send new probes from addons now. [1][2] For simpler use-cases that removes the need for setting up a custom ping.
I agree, this looks like some breakdown would be good.

For reading existing pings i'd like to talk about use-cases and how to best cover them.
This is a generally expensive operation (hitting the disk for each individual ping).

1: https://medium.com/georg-fritzsche/recording-new-telemetry-from-add-ons-61d194568212
2: https://www.a2p.it/wordpress/tech-stuff/mozilla/recording-telemetry-scalars-from-add-ons/
cc'ing in Matthew for lockbox support
Assignee: jhirsch → ntim.bugs
(In reply to Georg Fritzsche [:gfritzsche] from comment #27)

> Sending a plain ping is very valuable, but we can also send new probes from
> addons now. 

So, we can send these pings from *legacy* add-ons - but not WebExtensions. 

WebExtensions don't have access to Services.telemetry.{registerEvents,recordEvent}

If that's the API going forward, could we get WebExtension API wrappers for those calls?
Assignee: ntim.bugs → jhirsch
Jared and i talked about this and collected requirements for a Telemetry WebExtension API from the Shield, Normandy & Test Pilot teams.

Jared will work on this API in Q1 (cheers!).
The semantics of this API depend & impact directly on the Firefox Telemetry component, so my team can own this going forward (i'm not entirely clear how ownership plays out for WebExtension APIs though).

We broke the requirements down into two stages, with stage 1 being Jareds focus in Q1.
The API will mostly forward to Telemetry.

In stage 1, the Telemetry API will contain:
- canUpload() - is data upload enabled for Firefox?
- submitPing() - submitting a custom ping to Telemetry.
- Registering probes:
  - registerEvents() - registering new events off-train [1]
  - registerScalars() - registering new scalars off-train
- Recording probes:
  - scalarAdd(), scalarSet(), scalarSetMaximum() - record values into scalars
  - recordEvent() - recording events

Stage 2 will follow later and will contain:
- Marking active experiments.
- Recording into histograms (if there is a need for that).

1: https://medium.com/georg-fritzsche/recording-new-telemetry-from-add-ons-61d194568212
I've attached a work-in-progress patch. Three issues I'm not sure about:

1. The `canUpload` method seems to be declaring its return type correctly, but when I call it from the test webextension[1], it always returns undefined, not a boolean. What am I missing?

2. What do we want to do about errors emitted by the underlying Telemetry APIs? Do we want to pass them through, catch and convert the type and rethrow, or swallow them (possibly logging to console in nightly or if a dev-related pref is enabled)?
The test webextension[1], when you press its browser action button, calls the Telemetry APIs in a variety of incorrect ways. We can use that to ground the discussion.

3. Do I need to write tests for this API? I didn't see test files in the tree for other APIs.
Flags: needinfo?(aswan)
A note for observers from teams interested in this bug: I'm aiming to get this landed in the 60 timeframe. I'm not planning to seek uplift, but please needinfo addons folks if uplift is needed.

Also: gfritzsche and I met a few times to hash out the API details, as summarized in comment 21. The remaining issues are covered in my previous comment, but for those interested in a deeper dive, our meeting notes are available in this GDoc (currently moco-only, but we can open it up if contributors express interest): https://docs.google.com/document/d/1eV6FshjdbcTV3N477Da505Uv2aFiMOVOUk9XxLhd1Jo/edit
(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #33)
> I've attached a work-in-progress patch. Three issues I'm not sure about:
> 
> 1. The `canUpload` method seems to be declaring its return type correctly,
> but when I call it from the test webextension[1], it always returns
> undefined, not a boolean. What am I missing?

That function (and all the rest I presume) should be declared async in the schema.
There are some notes about that here:
https://firefox-source-docs.mozilla.org/toolkit/components/extensions/webextensions/background.html#javascript-apis

> 2. What do we want to do about errors emitted by the underlying Telemetry
> APIs? Do we want to pass them through, catch and convert the type and
> rethrow, or swallow them (possibly logging to console in nightly or if a
> dev-related pref is enabled)?
> The test webextension[1], when you press its browser action button, calls
> the Telemetry APIs in a variety of incorrect ways. We can use that to ground
> the discussion.

The users of this API are probably better positioned to answer that but my gut instinct is that errors should probably be visible to callers.  They probably won't really be able to do much with them but I think its better for them to explicitly think about what happens in that case than to just quietly assume that everything succeeds.

> 3. Do I need to write tests for this API? I didn't see test files in the
> tree for other APIs.

Yes please.  Existing tests are in places like toolkit/components/extensions/test and browser/components/extensions/test.  I suspect this could be tested with xpcshell, but I don't know much about the telemetry side.

Thanks for doing this!  I'll take a closer look at the code shortly.
Flags: needinfo?(aswan)
Comment on attachment 8952470 [details]
Bug 1280234 - Expose Telemetry APIs to trusted WebExtensions;

https://reviewboard.mozilla.org/r/221690/#review227662

The basic structure here looks solid, thanks!  In addition to the notes from your needinfo (about testing and about making the functions async), a few additional things below.  We can talk about this on IRC if its easier, but our usual practice is to describe constraints on API inputs in the schema as much as possible.  The schema-generated wrappers will then take care of validating your inputs and the API implementation can focus on the mechanics of doing its thing instead of validating inputs.

Finally, I really only looked at this from the extensions point-of-view, a telemetry peer should probably also look it over.

::: toolkit/components/extensions/ext-telemetry.js:3
(Diff revision 1)
> +ChromeUtils.import("resource://gre/modules/TelemetryController.jsm");
> +ChromeUtils.import("resource://gre/modules/TelemetryUtils.jsm");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");

It looks like lazy module getters would work here?

::: toolkit/components/extensions/ext-telemetry.js:12
(Diff revision 1)
> +        send(message, options = {
> +          addClientId: true,
> +          addEnvironment: true
> +        }) {

The defaults here should be declared in the schema.

::: toolkit/components/extensions/ext-telemetry.js:23
(Diff revision 1)
> +          // Note: remove the ternary and direct pref check when
> +          // TelemetryController.canUpload() is implemented.

If this is going to land like this, please include a reference to a specific bug here.

::: toolkit/components/extensions/ext-telemetry.js:29
(Diff revision 1)
> +        // TODO: type check the name and value of scalar*, so that any console
> +        // warnings are emitted from this API, not the underlying Telemetry API?
> +        // or is that automatically handled by the schema?

The schema will check the basic type here (ie, since you declared it as a string, if an extension passes a different type, the wrappers will just throw an exception and this code will never be invoked).  If there are additional constraints, ideally they would also be expressed in the schema.

::: toolkit/components/extensions/ext-telemetry.js:43
(Diff revision 1)
> +        scalarSetMaximum(name, value) {
> +          Services.telemetry.scalarSetMaximum(name, value);
> +        },
> +        recordEvent(category, method, object, value, extra) {
> +          try {
> +            // TODO: what's the right way to invoke optional args? spread / rest?

I don't understand the issue here?

::: toolkit/components/extensions/ext-telemetry.js:46
(Diff revision 1)
> +            // TODO: how do we want to handle exceptions thrown by the underlying Telemetry APIs?
> +            // Is there an internal API that WebExtensions use to handle extensions?

I may be misunderstanding the question here but there are some notes about error handling at:
https://firefox-source-docs.mozilla.org/toolkit/components/extensions/webextensions/functions.html#implementing-a-function-in-the-main-process

::: toolkit/components/extensions/schemas/telemetry.json:10
(Diff revision 1)
> +      "parameters": [
> +        {
> +          "name": "message",
> +          "type": "object"
> +        },
> +        {
> +          "name": "options",
> +          "type": "object"
> +        }
> +      ]

Are there constraints on the objects passed in here?  (ie on what properties they may have, and what values those properties may have?)  If so, they should be declared here.
Attachment #8952470 - Flags: review?(aswan) → review-
Comment on attachment 8954575 [details]
Address review feedback & add xpcshell tests

https://reviewboard.mozilla.org/r/223530/#review229706


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/ext-telemetry.js:4
(Diff revision 1)
>  "use strict";
>  
> -ChromeUtils.import("resource://gre/modules/TelemetryController.jsm");
> -ChromeUtils.import("resource://gre/modules/TelemetryUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "TelemetryController",
> +  "resource://gre/modules/TelemetryController.jsm");

Error: Expected indentation of 31 spaces but found 2. [eslint: indent]

::: toolkit/components/extensions/ext-telemetry.js:6
(Diff revision 1)
>  "use strict";
>  
> -ChromeUtils.import("resource://gre/modules/TelemetryController.jsm");
> -ChromeUtils.import("resource://gre/modules/TelemetryUtils.jsm");
> -ChromeUtils.import("resource://gre/modules/Services.jsm");
> +ChromeUtils.defineModuleGetter(this, "TelemetryController",
> +  "resource://gre/modules/TelemetryController.jsm");
> +ChromeUtils.defineModuleGetter(this, "TelemetryUtils",
> +  "resource://gre/modules/TelemetryUtils.jsm");

Error: Expected indentation of 31 spaces but found 2. [eslint: indent]

::: toolkit/components/extensions/ext-telemetry.js:8
(Diff revision 1)
> -ChromeUtils.import("resource://gre/modules/TelemetryController.jsm");
> -ChromeUtils.import("resource://gre/modules/TelemetryUtils.jsm");
> -ChromeUtils.import("resource://gre/modules/Services.jsm");
> +ChromeUtils.defineModuleGetter(this, "TelemetryController",
> +  "resource://gre/modules/TelemetryController.jsm");
> +ChromeUtils.defineModuleGetter(this, "TelemetryUtils",
> +  "resource://gre/modules/TelemetryUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "Services",
> +  "resource://gre/modules/Services.jsm");

Error: Expected indentation of 31 spaces but found 2. [eslint: indent]
Comment on attachment 8954575 [details]
Address review feedback & add xpcshell tests

Followup commit, xpcshell tests are passing.

Questions:

- What's the right way to add followup commits so that `git mozreview push` correctly asks for review - should I include bug number and 'r?[reviewers]' in the message?

- (for gfritzsche) Do I need to set up a mock server for the |submitExternalPing| test, or is it fine as written?

- (for aswan) I can't figure out how to test that the mozillaAddons permission is required; if I include that permission in the test extension, an error is logged ("Stripping mozillaAddons permission from {UUID}"). If I just omit the permission from the test webextension, the API loads and the tests run ok. Do I need to worry about testing this?

- (for aswan) The schema is very minimal right now. I'd like to wait until v2 of the API to add detailed schema verification, probably with the help of Telemetry peers. Does that sound ok?

- (for aswan) I'm not able to test the failure mode for the Telemetry |recordEvent| method, because the Extension code seems to throw errors that I can't catch; not sure what I'm missing. If you install the test webextension[1], then look at the browser console and press the browser action button, you'll see the three incorrect |recordEvent| calls trigger three "An unexpected error occurred" errors, which are thrown from an undefined file. dxr suggests those are either ExtensionCommon.jsm or ExtensionContent.jsm, but I'm not sure what to do to catch them.

[1] https://github.com/6a68/telemetry-webext-api-test
Attachment #8954575 - Flags: review?(gfritzsche)
Attachment #8954575 - Flags: review?(aswan)
Attachment #8943813 - Attachment is obsolete: true
Comment on attachment 8954575 [details]
Address review feedback & add xpcshell tests

https://reviewboard.mozilla.org/r/223530/#review229710


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/ext-telemetry.js:4
(Diff revision 2)
>  "use strict";
>  
> -ChromeUtils.import("resource://gre/modules/TelemetryController.jsm");
> -ChromeUtils.import("resource://gre/modules/TelemetryUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "TelemetryController",
> +  "resource://gre/modules/TelemetryController.jsm");

Error: Expected indentation of 31 spaces but found 2. [eslint: indent]

::: toolkit/components/extensions/ext-telemetry.js:6
(Diff revision 2)
>  "use strict";
>  
> -ChromeUtils.import("resource://gre/modules/TelemetryController.jsm");
> -ChromeUtils.import("resource://gre/modules/TelemetryUtils.jsm");
> -ChromeUtils.import("resource://gre/modules/Services.jsm");
> +ChromeUtils.defineModuleGetter(this, "TelemetryController",
> +  "resource://gre/modules/TelemetryController.jsm");
> +ChromeUtils.defineModuleGetter(this, "TelemetryUtils",
> +  "resource://gre/modules/TelemetryUtils.jsm");

Error: Expected indentation of 31 spaces but found 2. [eslint: indent]

::: toolkit/components/extensions/ext-telemetry.js:8
(Diff revision 2)
> -ChromeUtils.import("resource://gre/modules/TelemetryController.jsm");
> -ChromeUtils.import("resource://gre/modules/TelemetryUtils.jsm");
> -ChromeUtils.import("resource://gre/modules/Services.jsm");
> +ChromeUtils.defineModuleGetter(this, "TelemetryController",
> +  "resource://gre/modules/TelemetryController.jsm");
> +ChromeUtils.defineModuleGetter(this, "TelemetryUtils",
> +  "resource://gre/modules/TelemetryUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "Services",
> +  "resource://gre/modules/Services.jsm");

Error: Expected indentation of 31 spaces but found 2. [eslint: indent]
Comment on attachment 8952470 [details]
Bug 1280234 - Expose Telemetry APIs to trusted WebExtensions;

https://reviewboard.mozilla.org/r/221688/#review230030

::: toolkit/components/extensions/schemas/telemetry.json:38
(Diff revision 4)
> +          "name": "message",
> +          "type": "any"
> +        },
> +        {
> +          "name": "options",
> +          "type": "any",

It looks like this should be `type: "object"` with the valid properties enumerated.  For example: https://searchfox.org/mozilla-central/source/toolkit/components/extensions/schemas/runtime.json#341-349

::: toolkit/components/extensions/schemas/telemetry.json:62
(Diff revision 4)
> +      }
> +    },
> +    {
> +      "name": "scalarAdd",
> +      "type": "function",
> +      "description": "TODO",

Please fill this in before landing (same with other instances below)

::: toolkit/components/extensions/schemas/telemetry.json:87
(Diff revision 4)
> +          "name": "name",
> +          "type": "string"
> +        },
> +        {
> +          "name": "value",
> +          "type": "any"

should this be integer?  or number?

::: toolkit/components/extensions/schemas/telemetry.json:149
(Diff revision 4)
> +          "name": "category",
> +          "type": "string"
> +        },
> +        {
> +          "name": "data",
> +          "type": "any"

Can we really pass any value in here?  I'm kind of torn here, we usually try to specify all constraints on inputs in the schema so arguments can be verified up front, but if the underlying telemetry method that this calls is going to validate its input, you can probably make a case for not duplicating all that logic here...
(same with the data parameter to registerEvents() below)

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:31
(Diff revision 4)
> +add_task(async function test_telemetry_scalar_add() {
> +  await run({
> +    backgroundScript: () => {
> +      browser.telemetry.scalarAdd("telemetry.test.unsigned_int_kind", 1);
> +      browser.test.notifyPass("scalar_add");
> +    },
> +    doneSignal: "scalar_add"
> +  });
> +});

can this test verify that the scalar was actually incremented?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:42
(Diff revision 4)
> +    doneSignal: "scalar_add"
> +  });
> +});
> +
> +add_task(async function test_telemetry_scalar_add_unknown_name() {
> +  let {messages} = await promiseConsoleOutput(async function() {

nit: since `run()` is async, the anonymous function here doesn't need to be async, you can just return the result of calling `run()` directly.

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:52
(Diff revision 4)
> +  messages = messages.filter(msg => /Unknown scalar/);
> +  equal(messages.length, 1, "Telemetry should throw if an unknown scalar is incremented");

I'm confused by this message, the API method doesn't throw here, it just logs an error to the console.  It does seem like throwing (or rejecting to be precise) would be ideal, but if that's impractical can you change the message here to be clear (ie something like "Telemetry should log an error when ...")

There are several similar instances below.

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:84
(Diff revision 4)
> +add_task(async function test_telemetry_scalar_set() {
> +  await run({
> +    backgroundScript: () => {
> +      browser.telemetry.scalarSet("telemetry.test.unsigned_int_kind", 1);
> +      browser.telemetry.scalarSet("telemetry.test.boolean_kind", true);
> +      browser.test.notifyPass("scalar_set");
> +    },
> +    doneSignal: "scalar_set"
> +  });
> +});

can we test that those two calls actually took effect?  (and similarly for other test cases below)

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:171
(Diff revision 4)
> +add_task(async function test_telemetry_register_scalars() {
> +  await run({
> +    backgroundScript: () => {
> +      browser.telemetry.registerScalars("telemetry.test.dynamic", {
> +        "webext": {
> +          kind: 0, // Ci.nsITelemetry.SCALAR_TYPE_COUNT

Does it make sense to expose this is a constant in browser.telemetry?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:186
(Diff revision 4)
> +
> +  const scalars = Services.telemetry.snapshotScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, true);
> +  equal(scalars["dynamic"]["telemetry.test.dynamic.webext"], 123);
> +});
> +
> +add_task(async function test_telemetry_snapshot_scalars() {

nit: should this be test_telemetry_register_events()?

::: toolkit/components/extensions/test/xpcshell/xpcshell-common.ini:79
(Diff revision 4)
>  [test_ext_storage_sync_crypto.js]
>  skip-if = os == "android"
>  [test_ext_storage_telemetry.js]
> +skip-if = os == "android"
> +[test_ext_telemetry.js]
>  skip-if = os == "android" # checking for telemetry needs to be updated: 1384923

this comment should be next to the skip-if line for test_ext_storage_telemetry.js (ie it should be two lines up)
Comment on attachment 8954575 [details]
Address review feedback & add xpcshell tests

https://reviewboard.mozilla.org/r/223530/#review230664

Great to see this happening!
This looks good overall from the Telemetry side, i have some smaller comments below.

::: toolkit/components/extensions/schemas/telemetry.json:49
(Diff revision 3)
> +          }
>          }
>        ]
>      },
>      {
>        "name": "canUpload",

I think this is missing test coverage currently?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:31
(Diff revision 3)
> +    permissions: [],
> +    doneSignal: "telemetry_permission"
> +  });
> +});
> +
> +add_task(async function test_telemetry_scalar_add() {

This file has extensive coverage, cheers!

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:149
(Diff revision 3)
> +    doneSignal: "scalar_set_maximum_illegal_value"
> +  });
> +});
> +
> +add_task(async function test_telemetry_record_event() {
> +  Services.telemetry.setEventRecordingEnabled("telemetry.test", true);

I think we need to expose this through the WebExtension API.
Otherwise WebExtensions can't enable built-in events as needed.

Could be a follow-up bug though?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:171
(Diff revision 3)
> +add_task(async function test_telemetry_register_scalars() {
> +  await run({
> +    backgroundScript: () => {
> +      browser.telemetry.registerScalars("telemetry.test.dynamic", {
> +        "webext": {
> +          kind: 0, // Ci.nsITelemetry.SCALAR_TYPE_COUNT

Is there a mechanism to expose these as constants through `browser.telemetry`?
Would that make sense?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:216
(Diff revision 3)
> +      browser.test.notifyPass("submit_ping");
> +    },
> +    doneSignal: "submit_ping"
> +  });
> +
> +  // TODO: is this ok? Or do we need to start up the mock server?

This is fine here.
We have test coverage for the sending of pings in the existing Telemetry test, so you can rely on that contract.
Attachment #8954575 - Flags: review?(gfritzsche)
Comment on attachment 8952470 [details]
Bug 1280234 - Expose Telemetry APIs to trusted WebExtensions;

clearing for now while Jared prepares a revised patch.
Attachment #8952470 - Flags: review?(aswan)
Comment on attachment 8954575 [details]
Address review feedback & add xpcshell tests

clearing for now while Jared prepares a revised patch.
Attachment #8954575 - Flags: review?(aswan)
Blocks: 1449694
Product: Toolkit → WebExtensions
Comment on attachment 8952470 [details]
Bug 1280234 - Expose Telemetry APIs to trusted WebExtensions;

https://reviewboard.mozilla.org/r/221690/#review267392
Attachment #8952470 - Flags: review?(jhirsch)
Comment on attachment 8952470 [details]
Bug 1280234 - Expose Telemetry APIs to trusted WebExtensions;

Updated at last! The schema and tests have been fleshed out quite a bit. Most of the Telemetry-related code is the same, but figured, given how old the original patch was, that a new quick review might make sense.
Attachment #8952470 - Flags: review?(jhirsch)
Attachment #8952470 - Flags: review?(gfritzsche)
Attachment #8952470 - Flags: review?(aswan)
Attachment #8954575 - Attachment is obsolete: true
Attachment #8952470 - Flags: review?(gfritzsche)
Comment on attachment 8952470 [details]
Bug 1280234 - Expose Telemetry APIs to trusted WebExtensions;

https://reviewboard.mozilla.org/r/221690/#review267478

A few nits, but looks good, thanks!

::: toolkit/components/extensions/jar.mn:34
(Diff revision 3)
>      content/extensions/parent/ext-protocolHandlers.js (parent/ext-protocolHandlers.js)
>      content/extensions/parent/ext-proxy.js (parent/ext-proxy.js)
>      content/extensions/parent/ext-runtime.js (parent/ext-runtime.js)
>      content/extensions/parent/ext-storage.js (parent/ext-storage.js)
>      content/extensions/parent/ext-tabs-base.js (parent/ext-tabs-base.js)
> +#ifndef ANDROID

is this going to be desktop-only indefinitely?  if so, it should probably just go in browser/components/extensions instead of toolkit

::: toolkit/components/extensions/parent/ext-telemetry.js:23
(Diff revision 3)
> +this.telemetry = class extends ExtensionAPI {
> +  getAPI(context) {
> +    return {
> +      telemetry: {
> +        submitPing(type, payload, options) {
> +          return new Promise((resolve, reject) => {

you shouldn't need the Promise here, you can just return synchronously or throw ExtensionError.  (same for many of the other methods below)

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:7
(Diff revision 3)
> +
> +ChromeUtils.import("resource://gre/modules/TelemetryArchive.jsm", this);
> +
> +function createExtension(backgroundScript, permissions) {
> +  let extensionData = {
> +    background: `(${backgroundScript})()`,

loadExtension() will do this for you, so just `background: backgroundScript` should do

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:34
(Diff revision 3)
> +});
> +
> +add_task(async function test_telemetry_scalar_add() {
> +  await run({
> +    backgroundScript: () => {
> +      browser.telemetry.scalarAdd("telemetry.test.unsigned_int_kind", 1)

Do you care about testing that this actually had an effect?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:184
(Diff revision 3)
> +  Services.telemetry.clearEvents();
> +});
> +
> +add_task(async function test_telemetry_register_scalars_string() {
> +  await run({
> +    backgroundScript: () => {

this should work fine if you make this async and then you can use await below instead of chaining promise handlers.
Attachment #8952470 - Flags: review?(aswan) → review+
Thanks for the review, :aswan! I just realized that I overlooked bug 1381099, so I'll expose that pref and r? once more when it's ready.
Blocks: 1381099
Comment on attachment 8952470 [details]
Bug 1280234 - Expose Telemetry APIs to trusted WebExtensions;

https://reviewboard.mozilla.org/r/221690/#review267822

Cheers, this looks good to me from the Telemetry side.
It's great to see comprehensive test coverage here.

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:40
(Diff revision 3)
> +        .then(() => {
> +          browser.test.notifyPass("scalar_add");
> +        });
> +    },
> +    doneSignal: "scalar_add",
> +  });

Can you also check that the scalar is now set to the right value?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:100
(Diff revision 3)
> +        .then(() => {
> +          browser.test.notifyPass("scalar_set");
> +        });
> +    },
> +    doneSignal: "scalar_set",
> +  });

Test that the scalar is now set to the right value?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:100
(Diff revision 3)
> +        .then(() => {
> +          browser.test.notifyPass("scalar_set");
> +        });
> +    },
> +    doneSignal: "scalar_set",
> +  });

Test that the scalar is now set to the right value?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:129
(Diff revision 3)
> +        .then(() => {
> +          browser.test.notifyPass("scalar_set_maximum");
> +        });
> +    },
> +    doneSignal: "scalar_set_maximum",
> +  });

Test that the scalar is now set to the right value?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:129
(Diff revision 3)
> +        .then(() => {
> +          browser.test.notifyPass("scalar_set_maximum");
> +        });
> +    },
> +    doneSignal: "scalar_set_maximum",
> +  });

Test that the scalar is now set to the right value?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:129
(Diff revision 3)
> +        .then(() => {
> +          browser.test.notifyPass("scalar_set_maximum");
> +        });
> +    },
> +    doneSignal: "scalar_set_maximum",
> +  });

Test that the scalar is now set to the right value?

::: toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js:315
(Diff revision 3)
> +  });
> +
> +  let pings = await TelemetryArchive.promiseArchivedPingList();
> +  equal(pings.length, 1);
> +  equal(pings[0].type, "webext-test");
> +});

Does it make sense to add a test for `canUpload`?
Attachment #8952470 - Flags: review?(gfritzsche) → review+
Comment on attachment 8952470 [details]
Bug 1280234 - Expose Telemetry APIs to trusted WebExtensions;

https://reviewboard.mozilla.org/r/221690/#review267478

> is this going to be desktop-only indefinitely?  if so, it should probably just go in browser/components/extensions instead of toolkit

We don't have plans for Test Pilot experiments on Android, but I guess there's no reason not to enable this API there. 

I tried enabling on Android and pushed to Try. Not sure if these results look ok; I ran the xpcshell tests on all the Android platforms listed in Try Chooser, but there are a couple oranges and a failed build. What do you think?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cda9c6a63efe6c00576d0ff7e31d6f870d70d55

> Do you care about testing that this actually had an effect?

Yes; added checks for the scalar values.
Fixed up the code, following the review comments. Any final feedback is welcome, otherwise I'll land this patch as-is.

Should I worry about the Try results on Android (see the link in comment 55 above)?
Flags: needinfo?(aswan)
If this patch looks ok, I'll squash the commits into one before landing.
(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #56)
> Should I worry about the Try results on Android (see the link in comment 55
> above)?

Discussed briefly on IRC, but the short answer is "yes" :)
Flags: needinfo?(aswan)
:-) Fixed the stray #ifndef (thanks, :ntim!) and re-pushed to Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ded87095a05915a70c41cad4e0811d52aaa1c10

I'm out Mon-Tues, but will squash and land when I'm back.
Do I need to do anything special in the submitPing test to get `submitExternalPing` working on Android?

Looking at the failures in the latest try build, the ping doesn't seem to be recorded (TelemetryArchive.promiseArchivedPingList() returns an empty list, not a single ping as expected).
Flags: needinfo?(gfritzsche)
The `submitExternalPing` situation may be slightly broken and there is no general good support for this.
Similarly for recording the standard probes; they're not recorded on release etc.

I'd recommend to simply not support Android at this point, as the current Android Gecko Telemetry state is not something anyone should build new features on.
We're looking into a rebooted Mobile Telemetry SDK (towards Q1 2019) and could revisit this in this context.
Flags: needinfo?(gfritzsche)
Attachment #8997214 - Attachment is obsolete: true
Attachment #8997215 - Attachment is obsolete: true
Attachment #8997216 - Attachment is obsolete: true
Attachment #8997217 - Attachment is obsolete: true
Attachment #8997218 - Attachment is obsolete: true
Pushed by jhirsch@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3dc7979622c3
Expose Telemetry APIs to trusted WebExtensions; r=aswan,gfritzsche
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Backed out for xpcshell failures e.g.test_ext_contentscript_restrictSchemes.js

backout: https://hg.mozilla.org/integration/autoland/rev/09fe744984bc6f548ae4b9f2e09ae44773215923


push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3dc7979622c3ca97221db9320efe1d2c9ca76cb8

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193402839&repo=autoland&lineNumber=1360

[task 2018-08-11T01:03:48.306Z] 01:03:48     INFO -  TEST-START | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_restrictSchemes.js
[task 2018-08-11T01:04:13.970Z] 01:04:13  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_restrictSchemes.js | xpcshell return code: 0
[task 2018-08-11T01:04:13.970Z] 01:04:13     INFO -  TEST-INFO took 25664ms
[task 2018-08-11T01:04:13.971Z] 01:04:13     INFO -  >>>>>>>
[task 2018-08-11T01:04:13.972Z] 01:04:13     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_restrictSchemes.js | xpcw: cd /sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell
[task 2018-08-11T01:04:13.973Z] 01:04:13     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_restrictSchemes.js | xpcw: xpcshell -r /sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/sdcard/tests/xpc/m"; -f /sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head.js", "/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head_telemetry.js", "/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head_storage.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_ext_contentscript_restrictSchemes.js"]; -e const _TEST_NAME = "xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_restrictSchemes.js" -e _execute_test(); quit(0);
[task 2018-08-11T01:04:13.974Z] 01:04:13     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_restrictSchemes.js | [814, Unnamed thread 46b0d080] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 209
[task 2018-08-11T01:04:13.975Z] 01:04:13     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_restrictSchemes.js | [814, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2717
[task 2018-08-11T01:04:13.975Z] 01:04:13     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-08-11T01:04:13.976Z] 01:04:13     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-08-11T01:04:13.976Z] 01:04:13     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-08-11T01:04:13.977Z] 01:04:13     INFO -  running event loop
[task 2018-08-11T01:04:13.977Z] 01:04:13     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_restrictSchemes.js | Starting check_remote
[task 2018-08-11T01:04:13.977Z] 01:04:13     INFO -  (xpcshell/head.js) | test check_remote pending (2)
[task 2018-08-11T01:04:13.979Z] 01:04:13     INFO -  TEST-PASS | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_restrictSchemes.js | check_remote - [check_remote : 1] useRemoteWebExtensions matches - false == false
Flags: needinfo?(jhirsch)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Talked this over with :aswan on IRC. The plan:

1) move from toolkit/ into browser/, since enabling on android seems relatively far off in the future

2) remove the 'returns' properties from the schema, as they are likely the cause of the debug build errors: https://searchfox.org/mozilla-central/source/toolkit/components/extensions/Schemas.jsm#2011-2014

3) kick off another Try run and see if tests pass
Flags: needinfo?(jhirsch)
After later discussion, moved the code back to toolkit, and hacked a way to disable loading / running tests on android.

Looks like we have a clean Try run at last:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a87493f5bc6513f1ebd617c4a88a5bfeb3a76983
Pushed by jhirsch@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d3f55ddb602b
Expose Telemetry APIs to trusted WebExtensions; r=aswan,gfritzsche
https://hg.mozilla.org/mozilla-central/rev/d3f55ddb602b
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Can you please add some STRs to this issue and also attach a test webextension if possible? If no QA is needed please add "qe-verify-" flag
Flags: needinfo?(jhirsch)
What did you have in mind for a test webextension? Do you want fields with a submit button for each API, to test edge cases? Is there an example I can look at?
Flags: needinfo?(jhirsch) → needinfo?(vcarciu)
Since the API has automated tests, I would recommend just setting qe-verify-, I don't think manual testing would add much here.
I will wait for a confirmation on https://bugzilla.mozilla.org/show_bug.cgi?id=1280234#c78 before responding to to https://bugzilla.mozilla.org/show_bug.cgi?id=1280234#c77 .
Flags: needinfo?(vcarciu)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: