Add GeckoView API for Telemetry

RESOLVED FIXED in Firefox 61

Status

enhancement
P1
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: snorp, Assigned: esawin)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

Details

(Whiteboard: [geckoview:klar])

Attachments

(3 attachments, 10 obsolete attachments)

18.66 KB, patch
esawin
: review+
Details | Diff | Splinter Review
7.41 KB, patch
esawin
: review+
Details | Diff | Splinter Review
3.82 KB, patch
esawin
: review+
Details | Diff | Splinter Review
This should really be two different bugs, but it shouldn't be too bad:

1) Apps should be able to opt in to Telemetry via GeckoSessionSettings
2) Apps shoudl be able to change the Telemetry product name via GeckoSessionSettings (globally)
Eugen, we need this for the Klar effort, can you take a look?
Flags: needinfo?(esawin)
Whiteboard: [geckoview:klar]
(Assignee)

Comment 2

a year ago
1. Enabling and disabling of Gecko telemetry is controlled through the "toolkit.telemetry.enabled" pref.
We can expose an app-wide boolean setting to control that pref. 

2. The app name and the rest of the app info is set statically or read from application.ini at Gecko startup.
To allow for GV to dynamically set app info, we would need to rewrite the mechanics around gAppData usage, and I don't think we want that.
A less invasive solution would be expanding the internal Gecko Telemetry API to allow for app info override at runtime. Chris, is that feasible and what would be the best way to achieve that?
Flags: needinfo?(esawin) → needinfo?(chutten)
(In reply to Eugen Sawin [:esawin] from comment #2)
> 1. Enabling and disabling of Gecko telemetry is controlled through the
> "toolkit.telemetry.enabled" pref.
> We can expose an app-wide boolean setting to control that pref. 

Yeah, this seems straightforward.

> 
> 2. The app name and the rest of the app info is set statically or read from
> application.ini at Gecko startup.
> To allow for GV to dynamically set app info, we would need to rewrite the
> mechanics around gAppData usage, and I don't think we want that.
> A less invasive solution would be expanding the internal Gecko Telemetry API
> to allow for app info override at runtime. Chris, is that feasible and what
> would be the best way to achieve that?

We could allow an environment variable to clobber the application.ini value. GeckoThread would set this from a GeckoSession.preload() argument. I think we'll need to rework preload() at that point, though, because it's getting crazy.
(In reply to Eugen Sawin [:esawin] from comment #2)
> 1. Enabling and disabling of Gecko telemetry is controlled through the
> "toolkit.telemetry.enabled" pref.

This is not 100% true. Telemetry preferences are a bit of a mess: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/internals/preferences.html

toolkit.telemetry.enabled is the "Telemetry Off Switch" iff toolkit.telemetry.unified is false. (which it generally is on Android)

So for GeckoView's initial purposes this might not matter, and I'm just typing to hear myself talk. But it might be a good idea to turn off both toolkit.telemetry.enabled _and_ datareporting.healthreport.uploadEnabled... and be aware that toolkit.telemetry.enabled might be locked on some platforms.

> 2. The app name and the rest of the app info is set statically or read from
> application.ini at Gecko startup.
> To allow for GV to dynamically set app info, we would need to rewrite the
> mechanics around gAppData usage, and I don't think we want that.
> A less invasive solution would be expanding the internal Gecko Telemetry API
> to allow for app info override at runtime. Chris, is that feasible and what
> would be the best way to achieve that?

We're always amenable to increasing capabilities, so long as they're tested and maintained :)

That being said, Telemetry isn't the only consumer of Services.appInfo.name, so it might make sense for GV to find a way to change it at the source: https://searchfox.org/mozilla-central/search?q=Services.appinfo.name

Have you had meetings about how you want GV application Telemetry to be handled? This sounds like the sort of detailed discussion that we should have with multiple parties in a room. Not sure if "let's use applicationName for identification" is the result of such a discussion or the first draft of such a plan?
Flags: needinfo?(chutten) → needinfo?(snorp)
(In reply to Chris H-C :chutten from comment #4)
> Have you had meetings about how you want GV application Telemetry to be
> handled? This sounds like the sort of detailed discussion that we should
> have with multiple parties in a room. Not sure if "let's use applicationName
> for identification" is the result of such a discussion or the first draft of
> such a plan?

This was a first draft. Georg Fritzsche reached out to me about the mobile Telemetry plans for 2018. I've scheduled a follow-up meeting with him and Frank Bertsch to discuss GeckoView.
Let's add product info via Telemetry as well so that we can differentiate e.g. crow vs klar load times.
We're waiting for Frank Bertsch to update the Telemetry team's mobile plan for 2018:

https://docs.google.com/document/d/16GzPcTo0IykiNRWI565FKWn0lmQryHTNa6Y7rAN7K_E/

In our meeting last week with Frank, we decided that the app will be responsible for sending its own application-level telemetry and that GeckoView will be responsible for sending its own telemetry (performance data and crash pings). GeckoView will need an API to ask the app when is a good time to submit telemetry reports. GeckoView's environment telemetry will probably want to mention the app name so we can filter telemetry on the server.
We were suggesting to have GeckoView reuse the existing Telemetry core to record and store data, but have the application take care of assembling and sending pings.

We updated the 2018 mobile plan:
https://docs.google.com/document/d/16GzPcTo0IykiNRWI565FKWn0lmQryHTNa6Y7rAN7K_E/

And expanded on the GeckoView details here:
https://docs.google.com/document/d/1k0VWVLk2Ao_MMoPgCDXyZR2pLj74SvHgSmo5yuUBJCo/
(Assignee)

Updated

a year ago
Depends on: 1445420
Clearing NI here as I think we (mostly?) have a good idea of how to proceed here.
Flags: needinfo?(snorp)
(Assignee)

Updated

a year ago
Blocks: 1452550
(Assignee)

Comment 10

a year ago
These are the Java parts of the API implementation.

We could decide the fate of GeckoSession.Response at this point, since it doesn't serve the case well when you have to signal a processing error.
Attachment #8967803 - Flags: review?(snorp)
Attachment #8967803 - Flags: review?(nchen)
(Assignee)

Comment 11

a year ago
These are the JS parts.

The GeckoViewTelemetry module is not here to stay, I  will move the implementation into GeckoViewTelemetryController (bug 1452551) before final review from Georg.
Attachment #8967805 - Flags: review?(nchen)
(Assignee)

Updated

a year ago
Depends on: 1452551
Comment on attachment 8967803 [details] [diff] [review]
0001-Bug-1437551-1.0-Add-GeckoRuntime-telemetry-API.-r-sn.patch

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

I think we should fix onError() somehow, otherwise looks fine

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/TelemetrySession.java
@@ +37,5 @@
> +    // Match with GeckoViewTelemetry.
> +    public static final int SNAPSHOT_TYPE_HISTOGRAMS = 1 << 0;
> +    public static final int SNAPSHOT_TYPE_KEYED_HISTOGRAMS = 1 << 1;
> +    public static final int SNAPSHOT_TYPE_SCALARS = 1 << 2;
> +    public static final int SNAPSHOT_TYPE_KEYED_SCALARS = 1 << 3;

You should add SNAPSHOT_TYPE_ALL here, and use it below. That way things don't get out of sync if you add another type later.

@@ +42,5 @@
> +
> +
> +    /* package */ TelemetrySession(final @NonNull GeckoRuntime runtime) {
> +        mRuntime = runtime;
> +        mEventDispatcher = EventDispatcher.getInstance();

We only use this in one spot so it doesn't seem like we really need to hold this. No harm either, of course.

@@ +60,5 @@
> +        }
> +        return null;
> +    }
> +
> +    // We might want to move a variant of this into its own file and reuse it.

Yeah, maybe we should move GeckoSession.Response to org.mozilla.geckoview.Response? GeckoResponse?

@@ +77,5 @@
> +         * Called when processing has ended in an error.
> +         *
> +         * @param error The error details object.
> +         */
> +        void onError(Object error);

Should this be an Exception or Throwable? What can we do with the error object currently?

@@ +129,5 @@
> +                public void sendError(final Object error) {
> +                    Log.e(LOGTAG, "getSnapshots failed: " + error);
> +                    callback.onError(error);
> +                }
> +            }); 

whitespace
Attachment #8967803 - Flags: review?(snorp) → review+
Comment on attachment 8967803 [details] [diff] [review]
0001-Bug-1437551-1.0-Add-GeckoRuntime-telemetry-API.-r-sn.patch

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

f+ mostly for JSON usage.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +161,5 @@
> +     * Return a new telemetry session to access GeckoRuntime telemetry.
> +     *
> +     * @return A new telemetry session object.
> +     */
> +    public TelemetrySession getTelemetrySession() {

My preference is `public RuntimeTelemetry getTelemetry()` because

1) We already use the word "session" to mean something else.
2) It follows the trend of naming session/runtime sub-objects "SessionFoo" or "RuntimeBar".
3) `getTelemetry` is more concise. `getTelemetrySession` doesn't convey more information IMO

@@ +162,5 @@
> +     *
> +     * @return A new telemetry session object.
> +     */
> +    public TelemetrySession getTelemetrySession() {
> +        return new TelemetrySession(this);

We should only create this once.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/TelemetrySession.java
@@ +49,5 @@
> +    private static JSONObject toJsonObject(final Object obj) {
> +        Object json = null;
> +
> +        try {
> +            json = new JSONTokener(obj.toString()).nextValue();

Just use `return new JSONObject(obj.toString());`, no need for `JSONTokener`.

@@ +77,5 @@
> +         * Called when processing has ended in an error.
> +         *
> +         * @param error The error details object.
> +         */
> +        void onError(Object error);

Similar to snorp's comment, what should the consumer do with `Object error`? Or `onError()` in general? For GeckoSession.Response, we call `respond(null)` when an error occurs. I think that's okay. I'm not sure the consumer gains much through an `onError` call that they now must implement.

@@ +87,5 @@
> +     * @param clear Whether the retrieved snapshots should be cleared.
> +     * @param callback Used to return the async response.
> +     */
> +    public void getSnapshots(final boolean clear,
> +          final @NonNull Callback<GeckoBundle> callback) {

I prefer all parameters to be aligned to each other, whether the first parameter is in-line or not. So either move `clear` to its own line, or indent `callback` to align with `clear`.

@@ +116,5 @@
> +                public void sendSuccess(final Object result) {
> +                    final JSONObject json = toJsonObject(result);
> +
> +                    try {
> +                        final GeckoBundle bundle = GeckoBundle.fromJSONObject(json);

Why use JSON? Why not pass the bundle directly through `result`?
Attachment #8967803 - Flags: review?(nchen) → feedback+
Comment on attachment 8967805 [details] [diff] [review]
0002-Bug-1437551-2.0-Add-GeckoRuntime-telemetry-API-backe.patch

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

Looks okay, but I want to take another look after you get rid of the GeckoViewTelemetry module

::: mobile/android/chrome/geckoview/geckoview.js
@@ +84,5 @@
>                      "GeckoViewRemoteDebugger");
>    ModuleManager.add("resource://gre/modules/GeckoViewTrackingProtection.jsm",
>                      "GeckoViewTrackingProtection");
> +  ModuleManager.add("resource://gre/modules/GeckoViewTelemetry.jsm",
> +                    "GeckoViewTelemetry");

This should not be a module in geckoview.js, but I think that's what you meant in your comment.

::: mobile/android/modules/geckoview/GeckoViewTelemetry.jsm
@@ +1,1 @@
> +

Extra line

@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";

Not necessary

@@ +14,5 @@
> +  EventDispatcher: "resource://gre/modules/Messaging.jsm",
> +});
> +
> +XPCOMUtils.defineLazyServiceGetters(this, {
> +  Telemetry: ["@mozilla.org/base/telemetry;1", "nsITelemetry"],

Use `Services.telemetry` instead

@@ +32,5 @@
> +    type: "histograms",
> +    flag: (1 << 0),
> +    get: aClear => {
> +      return Telemetry.snapshotHistograms(
> +        Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT, false, aClear);

`get: aClear => Telemetry.snapshotHistograms(...)`

@@ +86,5 @@
> +          }
> +          snapshots[tel.type] = snapshot;
> +        }
> +
> +        aCallback.onSuccess(JSON.stringify(snapshots));

Pass `snapshots` directly instead of JSON.
Attachment #8967805 - Flags: review?(nchen) → feedback+
(Assignee)

Comment 15

a year ago
Attachment #8967803 - Attachment is obsolete: true
Attachment #8968358 - Flags: review?(nchen)
(Assignee)

Comment 16

a year ago
Addressed comments, will rebase on bug 1452551 before next review.
Attachment #8967805 - Attachment is obsolete: true
(Assignee)

Comment 17

a year ago
Moved our Response out of GeckoSession for reuse.
Since this is used as an app callback, I've changed response to onResponse.

I still would prefer to have dedicate success and failure callbacks for cases where the return values can differ (e.g. we could return some error state which can be localized by the app) or where null is an acceptable return value.

Error cases always need to be handled and checking for null values isn't better than having to implement another callback.
Attachment #8968361 - Flags: review?(snorp)
Attachment #8968361 - Flags: review?(nchen)
Comment on attachment 8968358 [details] [diff] [review]
0001-Bug-1437551-1.1-Add-GeckoRuntime-telemetry-API.-r-sn.patch

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

LGTM!

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +195,5 @@
>          PrefsHelper.setPref(name, value, /* flush */ false);
>      }
>  
> +    /**
> +     * Return a the telemetry controller to access GeckoRuntime telemetry.

Maybe "Return the telemetry object for this runtime."?

@@ +197,5 @@
>  
> +    /**
> +     * Return a the telemetry controller to access GeckoRuntime telemetry.
> +     *
> +     * @return A new telemetry session object.

"The telemetry object"

@@ +199,5 @@
> +     * Return a the telemetry controller to access GeckoRuntime telemetry.
> +     *
> +     * @return A new telemetry session object.
> +     */
> +    public RuntimeTelemetry getTelemetry() {

Assert UI thread.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/RuntimeTelemetry.java
@@ +55,5 @@
> +    public void getSnapshots(
> +          final boolean clear,
> +          final @NonNull Response<GeckoBundle> response) {
> +        getSnapshots(SNAPSHOT_HISTOGRAMS | SNAPSHOT_KEYED_HISTOGRAMS |
> +                     SNAPSHOT_SCALARS | SNAPSHOT_KEYED_SCALARS,

SNAPSHOT_ALL
Attachment #8968358 - Flags: review?(nchen) → review+
Comment on attachment 8968361 [details] [diff] [review]
0003-Bug-1437551-3.0-Move-generic-callback-out-of-GeckoSe.patch

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

I have a slight preference for `GeckoResponse` following the `<Parent><Child>` naming scheme. It also feels slightly awkward to have the consumer call `.onResponse(foo)` instead of `.respond(foo)`, but I don't care too much either way.
Attachment #8968361 - Flags: review?(nchen) → review+
(Assignee)

Comment 20

a year ago
(In reply to Jim Chen [:jchen] [:darchons] from comment #19)
> I have a slight preference for `GeckoResponse` following the
> `<Parent><Child>` naming scheme.

I feel like Response is rather a child with multiple parents (GeckoSession and RuntimeTelemetry for now) than a top level class. There is nothing Gecko-specific about it.

> It also feels slightly awkward to have the
> consumer call `.onResponse(foo)` instead of `.respond(foo)`, but I don't
> care too much either way.

Right, although it's now also used as a consumer callback, so either name can seem odd depending on the use case.
I don't hold a strong opinion on this though.
(Assignee)

Comment 21

a year ago
Attachment #8968358 - Attachment is obsolete: true
Attachment #8968543 - Flags: review+
(In reply to Eugen Sawin [:esawin] from comment #20)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #19)
> > I have a slight preference for `GeckoResponse` following the
> > `<Parent><Child>` naming scheme.
> 
> I feel like Response is rather a child with multiple parents (GeckoSession
> and RuntimeTelemetry for now) than a top level class. There is nothing
> Gecko-specific about it.

It's a direct API entry point (people will be importing it and instantiating it). That makes it top-level in my mind.
Comment on attachment 8968361 [details] [diff] [review]
0003-Bug-1437551-3.0-Move-generic-callback-out-of-GeckoSe.patch

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

I also agree GeckoResponse seems more fitting. Inner classes can drop the "Gecko", but I think we need to keep it here.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/Response.java
@@ +14,5 @@
> +     * Called when async processing has finished.
> +     *
> +     * @param value The value contained in the response.
> +     */
> +    void onResponse(T value);

Agree with jchen here, respond() seemed better to me.
Attachment #8968361 - Flags: review?(snorp) → review+
(Assignee)

Comment 24

a year ago
Renamed to GeckoResponse.respond.
Attachment #8969070 - Flags: review+
(Assignee)

Comment 25

a year ago
An extension adding some comments and dataset types to allow the app to opt-in/out into extended telemetry.

Will merge with patch 1 on r+.
Attachment #8968361 - Attachment is obsolete: true
Attachment #8969072 - Flags: review?(snorp)
Attachment #8969072 - Flags: review?(nchen)
(Assignee)

Comment 26

a year ago
Handle "GeckoView:TelemetrySnapshots" requests in GeckoViewTelemetryController.

Georg, do we also need to set the nsITelemetry.canRecord* attributes to let the app choose the dataset?
Attachment #8968359 - Attachment is obsolete: true
Attachment #8969077 - Flags: review?(nchen)
Attachment #8969077 - Flags: review?(gfritzsche)
Comment on attachment 8969077 [details] [diff] [review]
0003-Bug-1437551-2.2-Add-GeckoRuntime-telemetry-API-backe.patch

Moving this to Alessio who is focusing on the GeckoView support.
Attachment #8969077 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
(In reply to Eugen Sawin [:esawin] from comment #26)
> Created attachment 8969077 [details] [diff] [review]
> 0003-Bug-1437551-2.2-Add-GeckoRuntime-telemetry-API-backe.patch
> 
> Handle "GeckoView:TelemetrySnapshots" requests in
> GeckoViewTelemetryController.
> 
> Georg, do we also need to set the nsITelemetry.canRecord* attributes to let
> the app choose the dataset?
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 29

a year ago
Looks like I've refactored the type filtering out.
Attachment #8969077 - Attachment is obsolete: true
Attachment #8969077 - Flags: review?(nchen)
Attachment #8969077 - Flags: review?(alessio.placitelli)
Attachment #8969447 - Flags: review?(nchen)
Attachment #8969447 - Flags: review?(alessio.placitelli)
(In reply to Eugen Sawin [:esawin] from comment #26)
> Created attachment 8969077 [details] [diff] [review]
> 0003-Bug-1437551-2.2-Add-GeckoRuntime-telemetry-API-backe.patch
> 
> Handle "GeckoView:TelemetrySnapshots" requests in
> GeckoViewTelemetryController.
> 
> Georg, do we also need to set the nsITelemetry.canRecord* attributes to let
> the app choose the dataset?

With bug 1452551 landed, Telemetry flags are automatically set here [0], with the same behaviour documented for Firefox Desktop [1]. Basically, Telemetry takes care of setting the right flags while the application decides if pings can be sent or not.

Opt-in measurements are recorded for all pre-release channels and local developer builds (controlled by the channel name).
Opt-out measurements are always recorded.

I think you should not expose that API to the application. To be future proof, in case the application has to do any collection outside of Telemetry and wants to rely consistently with the Telemetry core, you should expose [2] |nsITelemetry.canRecordReleaseData| and |nsITelemetry.canRecordPrereleaseData|. Please note that these are read-only :)

[0] - https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/toolkit/components/telemetry/TelemetryUtils.jsm#211
[1] - https://medium.com/georg-fritzsche/data-preference-changes-in-firefox-58-2d5df9c428b5
[2] - https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/toolkit/components/telemetry/nsITelemetry.idl#290,304
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8969447 [details] [diff] [review]
0003-Bug-1437551-2.3-Add-GeckoRuntime-telemetry-API-backe.patch

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

Hey Eugen! This looks good to me with the few minor observations addressed. In addition to these, my only concern is with catching the potential exceptions from the |registerListener|. Please flag me again for a quick pass after these!

::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm
@@ +16,5 @@
>  GeckoViewUtils.initLogging("GeckoView.TelemetryController", this);
>  
>  var EXPORTED_SYMBOLS = ["GeckoViewTelemetryController"];
>  
> +// Match with RuntimeTelemetry.SNAPSHOT_* and nsITelemetry.idl.

nit: let's mention that one more more of these can be requested at time

@@ +42,5 @@
> +    flag: (1 << 3),
> +    get: (dataset, clear) => Services.telemetry.snapshotKeyedScalars(
> +                               dataset, clear)
> +  },
> +]

nit: missing trailing ";"

@@ +57,5 @@
>  
>      debug `setup - canRecordPrereleaseData ${Services.telemetry.canRecordPrereleaseData
>            }, canRecordReleaseData ${Services.telemetry.canRecordReleaseData}`;
> +
> +    EventDispatcher.instance.registerListener(this, [

Looks like this will throw (https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/mobile/android/modules/geckoview/Messaging.jsm#39) when ran in the content process. Would you mind wrapping it in a try/catch or checking that we're in the parent process before registering? Let's still wrap it in a try/catch block as I'm not sure if it could fail for other reasons.

@@ +62,5 @@
> +      "GeckoView:TelemetrySnapshots",
> +    ]);
> +  },
> +
> +  // Bundle event handler.

nit: we're trying to use jsdoc3 for comments in toolkit/components/telemetry/* (but I see you've been using that in GeckoVIew as well https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/mobile/android/modules/geckoview/Messaging.jsm#57-66 ). Would you mind documenting the parameters here? I'm especially interested in the aCallback

@@ +66,5 @@
> +  // Bundle event handler.
> +  onEvent(aEvent, aData, aCallback) {
> +    debug `onEvent: aEvent=${aEvent}, aData=${aData}`;
> +
> +    switch (aEvent) {

Since this `switch` only has one `case`, what about bailing out early `if (aEvent != "GeckoView:TelemetrySnapshots) {
 debug "Unexpected event ${aEvent}"
return;
}

// other code`
Attachment #8969447 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 32

a year ago
Addressed comments.
Attachment #8969447 - Attachment is obsolete: true
Attachment #8969447 - Flags: review?(nchen)
Attachment #8969647 - Flags: review?(nchen)
Attachment #8969647 - Flags: review?(alessio.placitelli)
Comment on attachment 8969647 [details] [diff] [review]
0003-Bug-1437551-2.4-Add-GeckoRuntime-telemetry-API-backe.patch

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

Thanks!
Attachment #8969647 - Flags: review?(alessio.placitelli) → review+
(In reply to Eugen Sawin [:esawin] from comment #32)
> Created attachment 8969647 [details] [diff] [review]
> 0003-Bug-1437551-2.4-Add-GeckoRuntime-telemetry-API-backe.patch
> 
> Addressed comments.

Hey Eugen, are you planning on adding test coverage to the GeckoViewController API somewhere?
Flags: needinfo?(esawin)
(Assignee)

Comment 35

a year ago
(In reply to Alessio Placitelli [:Dexter] from comment #34)
> (In reply to Eugen Sawin [:esawin] from comment #32)
> > Created attachment 8969647 [details] [diff] [review]
> > 0003-Bug-1437551-2.4-Add-GeckoRuntime-telemetry-API-backe.patch
> > 
> > Addressed comments.
> 
> Hey Eugen, are you planning on adding test coverage to the
> GeckoViewController API somewhere?

There are no plans yet as GV's API is a simple Java wrapper for the snapshot API, which is covered by Gecko tests.
We might need to add tests for the Android persistency layer though, once that lands?
Flags: needinfo?(esawin)
(In reply to Eugen Sawin [:esawin] from comment #35)
> (In reply to Alessio Placitelli [:Dexter] from comment #34)
> > (In reply to Eugen Sawin [:esawin] from comment #32)
> > > Created attachment 8969647 [details] [diff] [review]
> > > 0003-Bug-1437551-2.4-Add-GeckoRuntime-telemetry-API-backe.patch
> > > 
> > > Addressed comments.
> > 
> > Hey Eugen, are you planning on adding test coverage to the
> > GeckoViewController API somewhere?
> 
> There are no plans yet as GV's API is a simple Java wrapper for the snapshot
> API, which is covered by Gecko tests.
> We might need to add tests for the Android persistency layer though, once
> that lands?

Yes, we definitely need to do that for the persistency part :)
Comment on attachment 8969072 [details] [diff] [review]
0002-Bug-1437551-A.1-Merge-with-patch-1.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/RuntimeTelemetry.java
@@ +27,5 @@
>  
>      private final GeckoRuntime mRuntime;
>      private final EventDispatcher mEventDispatcher;
>  
> +    @IntDef({ DATASET_BASE, DATASET_EXTENDED })

It appears `IntDef` and `StringDef` should have `@Retention(SOURCE)` (I get compiler warnings about it), so we should probably add those eventually.

@@ +77,5 @@
>      /**
>       * Retrieve all telemetry snapshots.
>       *
> +     * @param dataset The dataset type to retreive.
> +     *                One of {@link RuntimeTelemetry.DATASET_BASE} flags.

> {@link RuntimeTelemetry.DATASET_BASE DATASET_*}

Otherwise the doc appears as "One of DATASET_BASE flags", which doesn't make sense.
Attachment #8969072 - Flags: review?(nchen) → review+
Comment on attachment 8969647 [details] [diff] [review]
0003-Bug-1437551-2.4-Add-GeckoRuntime-telemetry-API-backe.patch

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

::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm
@@ +63,5 @@
>            }, canRecordReleaseData ${Services.telemetry.canRecordReleaseData}`;
> +
> +    if (GeckoViewUtils.IS_PARENT_PROCESS) {
> +      try {
> +        EventDispatcher.instance.registerListener(this.handleSnapshotsRequest, [

> registerListener(this, ...

and rename `handleSnapshotsRequest` to `onEvent`, otherwise the value of `this` is not preserved inside `handleSnapshotsRequest`.
Attachment #8969647 - Flags: review?(nchen) → review+
Attachment #8969072 - Flags: review?(snorp) → review+
(Assignee)

Comment 39

a year ago
Addressed comments and merged patches.
Attachment #8968543 - Attachment is obsolete: true
Attachment #8969072 - Attachment is obsolete: true
Attachment #8970263 - Flags: review+
(Assignee)

Comment 40

a year ago
Addressed comments.
Attachment #8969647 - Attachment is obsolete: true
Attachment #8970264 - Flags: review+

Comment 41

a year ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a401bb9e1aa4
[1.3] Add GeckoRuntime telemetry API. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b780b06d2eb
[2.5] Add GeckoRuntime telemetry API backend. r=jchen,Dexter
https://hg.mozilla.org/integration/mozilla-inbound/rev/45ba3fb2d9cc
[3.1] Move generic callback out of GeckoSession. r=snorp,jchen

Comment 43

a year ago
Backout by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1752eb8f975
Backed out changeset 45ba3fb2d9cc for build bustages on Android /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java a=backout CLOSED TREE
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=45ba3fb2d9ccf3cadf6f5b77808bdf9e25a7e415&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&filter-classifiedState=unclassified

Failure log:
https://treeherder.mozilla.org/logviewer.html#?job_id=175159159&repo=mozilla-inbound&lineNumber=35505

[task 2018-04-23T18:44:19.064Z] 18:44:19     INFO -  :geckoview:mergeOfficialWithGeckoBinariesNoMinApiReleaseJniLibFolders
[task 2018-04-23T18:44:19.074Z] 18:44:19     INFO -  :geckoview:transformNativeLibsWithMergeJniLibsForOfficialWithGeckoBinariesNoMinApiRelease
[task 2018-04-23T18:44:19.084Z] 18:44:19     INFO -  :geckoview:transformNativeLibsWithSyncJniLibsForOfficialWithGeckoBinariesNoMinApiRelease
[task 2018-04-23T18:44:23.483Z] 18:44:23     INFO -  :geckoview:bundleOfficialWithGeckoBinariesNoMinApiRelease
[task 2018-04-23T18:44:23.987Z] 18:44:23     INFO -  :geckoview:javadocOfficialWithGeckoBinariesNoMinApiReleasePicked up JAVA_TOOL_OPTIONS: -Dfile.encoding=utf-8
[task 2018-04-23T18:44:24.471Z] 18:44:24     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:22: error: cannot find symbol
[task 2018-04-23T18:44:24.471Z] 18:44:24     INFO -  import org.mozilla.gecko.IGeckoEditableParent;
[task 2018-04-23T18:44:24.472Z] 18:44:24     INFO -                          ^
[task 2018-04-23T18:44:24.472Z] 18:44:24     INFO -    symbol:   class IGeckoEditableParent
[task 2018-04-23T18:44:24.472Z] 18:44:24     INFO -    location: package org.mozilla.gecko
[task 2018-04-23T18:44:24.502Z] 18:44:24     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionTextInput.java:10: error: cannot find symbol
[task 2018-04-23T18:44:24.502Z] 18:44:24     INFO -  import org.mozilla.gecko.IGeckoEditableParent;
[task 2018-04-23T18:44:24.502Z] 18:44:24     INFO -                          ^
[task 2018-04-23T18:44:24.502Z] 18:44:24     INFO -    symbol:   class IGeckoEditableParent
[task 2018-04-23T18:44:24.502Z] 18:44:24     INFO -    location: package org.mozilla.gecko
[task 2018-04-23T18:44:24.532Z] 18:44:24     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:16: error: cannot find symbol
[task 2018-04-23T18:44:24.532Z] 18:44:24     INFO -  import org.mozilla.gecko.IGeckoEditableChild;
[task 2018-04-23T18:44:24.532Z] 18:44:24     INFO -                          ^
[task 2018-04-23T18:44:24.532Z] 18:44:24     INFO -    symbol:   class IGeckoEditableChild
[task 2018-04-23T18:44:24.532Z] 18:44:24     INFO -    location: package org.mozilla.gecko
[task 2018-04-23T18:44:24.533Z] 18:44:24     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:17: error: cannot find symbol
[task 2018-04-23T18:44:24.533Z] 18:44:24     INFO -  import org.mozilla.gecko.IGeckoEditableParent;
[task 2018-04-23T18:44:24.533Z] 18:44:24     INFO -                          ^
[task 2018-04-23T18:44:24.533Z] 18:44:24     INFO -    symbol:   class IGeckoEditableParent
[task 2018-04-23T18:44:24.533Z] 18:44:24     INFO -    location: package org.mozilla.gecko
[task 2018-04-23T18:44:24.544Z] 18:44:24     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:53: error: package IGeckoEditableParent does not exist
[task 2018-04-23T18:44:24.544Z] 18:44:24     INFO -      extends IGeckoEditableParent.Stub
[task 2018-04-23T18:44:24.544Z] 18:44:24     INFO -                                  ^
[task 2018-04-23T18:44:24.544Z] 18:44:24     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditableChild.java:24: error: cannot find symbol
[task 2018-04-23T18:44:24.545Z] 18:44:24     INFO -  public final class GeckoEditableChild extends JNIObject implements IGeckoEditableChild {
[task 2018-04-23T18:44:24.545Z] 18:44:24     INFO -                                                                     ^
[task 2018-04-23T18:44:24.545Z] 18:44:24     INFO -    symbol: class IGeckoEditableChild
[task 2018-04-23T18:44:24.545Z] 18:44:24     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:577: error: cannot find symbol
[task 2018-04-23T18:44:24.545Z] 18:44:24     INFO -          public native void attachEditable(IGeckoEditableParent parent,
[task 2018-04-23T18:44:24.545Z] 18:44:24     INFO -                                            ^
[task 2018-04-23T18:44:24.545Z] 18:44:24     INFO -    symbol:   class IGeckoEditableParent
[task 2018-04-23T18:44:24.545Z] 18:44:24     INFO -    location: class Window
[task 2018-04-23T18:44:24.552Z] 18:44:24     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:76: error: cannot find symbol
[task 2018-04-23T18:44:24.552Z] 18:44:24     INFO -      /* package */ IGeckoEditableChild mDefaultChild; // Used by IC thread.
[task 2018-04-23T18:44:24.553Z] 18:44:24     INFO -                    ^
[task 2018-04-23T18:44:24.553Z] 18:44:24     INFO -    symbol:   class IGeckoEditableChild
[task 2018-04-23T18:44:24.553Z] 18:44:24     INFO -    location: class GeckoEditable
[task 2018-04-23T18:44:24.553Z] 18:44:24     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:78: error: cannot find symbol
[task 2018-04-23T18:44:24.553Z] 18:44:24     INFO -      /* package */ IGeckoEditableChild mFocusedChild; // Used by IC thread.
[task 2018-04-23T18:44:24.553Z] 18:44:24     INFO -                    ^
[task 2018-04-23T18:44:24.553Z] 18:44:24     INFO -    symbol:   class IGeckoEditableChild
[task 2018-04-23T18:44:24.554Z] 18:44:24     INFO -    location: class GeckoEditable
[task 2018-04-23T18:44:24.555Z] 18:44:24     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:108: error: cannot find symbol
[task 2018-04-23T18:44:24.555Z] 18:44:24     INFO -      private void onKeyEvent(final IGeckoEditableChild child, KeyEvent event, int action,
[task 2018-04-23T18:44:24.555Z] 18:44:24     INFO -                                    ^
[task 2018-04-23T18:44:24.555Z] 18:44:24     INFO -    symbol:   class IGeckoEditableChild
[task 2018-04-23T18:44:24.555Z] 18:44:24     INFO -    location: class GeckoEditable
[task 2018-04-23T18:44:24.556Z] 18:44:24     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:626: error: cannot find symbol
[task 2018-04-23T18:44:24.556Z] 18:44:24     INFO -      public void setDefaultEditableChild(final IGeckoEditableChild child) {
[task 2018-04-23T18:44:24.556Z] 18:44:24     INFO -                                                ^
Flags: needinfo?(esawin)
(Assignee)

Comment 45

a year ago
It's a known issue, not related to this patches: bug 1448095.
Flags: needinfo?(esawin)

Comment 46

a year ago
Backout by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5724b08c3cbd
Backed out 2 changesets for build bustages on Android /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java a=backout CLOSED TREE

Comment 47

a year ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5582b64d4a8b
[1.4] Add GeckoRuntime telemetry API. r=snorp,jchen CLOSED TREE
https://hg.mozilla.org/integration/mozilla-inbound/rev/6badc13e1d21
[2.6] Add GeckoRuntime telemetry API backend. r=jchen,Dexter CLOSED TREE
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f30bed54236
[3.2] Move generic callback out of GeckoSession. r=snorp,jchen CLOSED TREE
Backed out because we're waiting for patches to be landed: https://bugzilla.mozilla.org/show_bug.cgi?id=1448095 

backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/cac46e410f9911d8748d8366c7ec939c71cebbee
pushes with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2f30bed5423644f54c7d8ef3115427f3f6e52382
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=781457239e63258ea069ccdd16ebaadb6c5fa33b

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175192502&repo=mozilla-inbound

[task 2018-04-23T21:50:39.810Z] 21:50:39     INFO -  :geckoview:mergeOfficialWithGeckoBinariesNoMinApiReleaseJniLibFolders
[task 2018-04-23T21:50:39.820Z] 21:50:39     INFO -  :geckoview:transformNativeLibsWithMergeJniLibsForOfficialWithGeckoBinariesNoMinApiRelease
[task 2018-04-23T21:50:39.830Z] 21:50:39     INFO -  :geckoview:transformNativeLibsWithSyncJniLibsForOfficialWithGeckoBinariesNoMinApiRelease
[task 2018-04-23T21:50:44.219Z] 21:50:44     INFO -  :geckoview:bundleOfficialWithGeckoBinariesNoMinApiRelease
[task 2018-04-23T21:50:44.753Z] 21:50:44     INFO -  :geckoview:javadocOfficialWithGeckoBinariesNoMinApiReleasePicked up JAVA_TOOL_OPTIONS: -Dfile.encoding=utf-8
[task 2018-04-23T21:50:45.177Z] 21:50:45     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:22: error: cannot find symbol
[task 2018-04-23T21:50:45.177Z] 21:50:45     INFO -  import org.mozilla.gecko.IGeckoEditableParent;
[task 2018-04-23T21:50:45.177Z] 21:50:45     INFO -                          ^
[task 2018-04-23T21:50:45.177Z] 21:50:45     INFO -    symbol:   class IGeckoEditableParent
[task 2018-04-23T21:50:45.177Z] 21:50:45     INFO -    location: package org.mozilla.gecko
[task 2018-04-23T21:50:45.197Z] 21:50:45     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionTextInput.java:10: error: cannot find symbol
[task 2018-04-23T21:50:45.197Z] 21:50:45     INFO -  import org.mozilla.gecko.IGeckoEditableParent;
[task 2018-04-23T21:50:45.197Z] 21:50:45     INFO -                          ^
[task 2018-04-23T21:50:45.197Z] 21:50:45     INFO -    symbol:   class IGeckoEditableParent
[task 2018-04-23T21:50:45.197Z] 21:50:45     INFO -    location: package org.mozilla.gecko
[task 2018-04-23T21:50:45.227Z] 21:50:45     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:16: error: cannot find symbol
[task 2018-04-23T21:50:45.228Z] 21:50:45     INFO -  import org.mozilla.gecko.IGeckoEditableChild;
[task 2018-04-23T21:50:45.228Z] 21:50:45     INFO -                          ^
[task 2018-04-23T21:50:45.228Z] 21:50:45     INFO -    symbol:   class IGeckoEditableChild
[task 2018-04-23T21:50:45.228Z] 21:50:45     INFO -    location: package org.mozilla.gecko
[task 2018-04-23T21:50:45.228Z] 21:50:45     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:17: error: cannot find symbol
[task 2018-04-23T21:50:45.228Z] 21:50:45     INFO -  import org.mozilla.gecko.IGeckoEditableParent;
[task 2018-04-23T21:50:45.228Z] 21:50:45     INFO -                          ^
[task 2018-04-23T21:50:45.231Z] 21:50:45     INFO -    symbol:   class IGeckoEditableParent
[task 2018-04-23T21:50:45.231Z] 21:50:45     INFO -    location: package org.mozilla.gecko
[task 2018-04-23T21:50:45.231Z] 21:50:45     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:53: error: package IGeckoEditableParent does not exist
[task 2018-04-23T21:50:45.231Z] 21:50:45     INFO -      extends IGeckoEditableParent.Stub
[task 2018-04-23T21:50:45.231Z] 21:50:45     INFO -                                  ^
[task 2018-04-23T21:50:45.231Z] 21:50:45     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditableChild.java:24: error: cannot find symbol
[task 2018-04-23T21:50:45.231Z] 21:50:45     INFO -  public final class GeckoEditableChild extends JNIObject implements IGeckoEditableChild {
[task 2018-04-23T21:50:45.231Z] 21:50:45     INFO -                                                                     ^
[task 2018-04-23T21:50:45.231Z] 21:50:45     INFO -    symbol: class IGeckoEditableChild
[task 2018-04-23T21:50:45.232Z] 21:50:45     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:577: error: cannot find symbol
[task 2018-04-23T21:50:45.232Z] 21:50:45     INFO -          public native void attachEditable(IGeckoEditableParent parent,
[task 2018-04-23T21:50:45.232Z] 21:50:45     INFO -                                            ^
[task 2018-04-23T21:50:45.232Z] 21:50:45     INFO -    symbol:   class IGeckoEditableParent
[task 2018-04-23T21:50:45.232Z] 21:50:45     INFO -    location: class Window
[task 2018-04-23T21:50:45.237Z] 21:50:45     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:76: error: cannot find symbol
[task 2018-04-23T21:50:45.237Z] 21:50:45     INFO -      /* package */ IGeckoEditableChild mDefaultChild; // Used by IC thread.
[task 2018-04-23T21:50:45.238Z] 21:50:45     INFO -                    ^
[task 2018-04-23T21:50:45.238Z] 21:50:45     INFO -    symbol:   class IGeckoEditableChild
[task 2018-04-23T21:50:45.238Z] 21:50:45     INFO -    location: class GeckoEditable
[task 2018-04-23T21:50:45.238Z] 21:50:45     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:78: error: cannot find symbol
[task 2018-04-23T21:50:45.239Z] 21:50:45     INFO -      /* package */ IGeckoEditableChild mFocusedChild; // Used by IC thread.
[task 2018-04-23T21:50:45.239Z] 21:50:45     INFO -                    ^
[task 2018-04-23T21:50:45.239Z] 21:50:45     INFO -    symbol:   class IGeckoEditableChild
[task 2018-04-23T21:50:45.239Z] 21:50:45     INFO -    location: class GeckoEditable
[task 2018-04-23T21:50:45.239Z] 21:50:45     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:108: error: cannot find symbol
[task 2018-04-23T21:50:45.239Z] 21:50:45     INFO -      private void onKeyEvent(final IGeckoEditableChild child, KeyEvent event, int action,
[task 2018-04-23T21:50:45.240Z] 21:50:45     INFO -                                    ^
Flags: needinfo?(nchen)
Flags: needinfo?(nchen)
Actually, looking at the logs, the real failures were "Unresolved reference: Response" errors when building tests.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=175192504&repo=mozilla-inbound&lineNumber=35275-35291
(Assignee)

Comment 51

a year ago
(In reply to Jim Chen [:jchen] [:darchons] from comment #50)
> Actually, looking at the logs, the real failures were "Unresolved reference:
> Response" errors when building tests.
> 
> [1]
> https://treeherder.mozilla.org/logviewer.html#?job_id=175192504&repo=mozilla-
> inbound&lineNumber=35275-35291

Yeah, those didn't show up in the treeherder failure summary, will fix before re-landing.

Comment 53

a year ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6aad40b3d02
[1.4] Add GeckoRuntime telemetry API. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/db1371bac45d
[2.6] Add GeckoRuntime telemetry API backend. r=jchen,Dexter
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0bde59038e
[3.2] Move generic callback out of GeckoSession. r=snorp,jchen

Updated

5 months ago
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 61 → mozilla61
You need to log in before you can comment on or make changes to this bug.