1269035 - Telemetry reuploader follow-up: try to re-use HTTP connection

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

via bug 1243585 comment 58:
 * Each ping sent up creates a new HTTP connection because I didn't change the existing BaseResource code. This caused me to get rate limited by my test server (and the same would probably happen from our telemetry server). We should try to combine them (if BaseResource makes this feasible, otherwise it's better as a follow-up)

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from bug 1243585 comment #59)
> Actually, it's unclear the telemetry server would accept multiple pings over
> the same connection.
> 
> Georg, can the telemetry server accept multiple pings over the same HTTP
> connection, or do I have to reconnect to send another ping? Also, is the
> telemetry server rate limited? If so, do you know under which metrics?

Comment 2

[Georg Fritzsche [:gfritzsche]]

Forwarding to mreid, who can answer this better.

Comment 3

[Mark Reid [:mreid]]

(In reply to Michael Comella (:mcomella) from comment #1)
> (In reply to Michael Comella (:mcomella) from bug 1243585 comment #59)
> > Georg, can the telemetry server accept multiple pings over the same HTTP
> > connection, or do I have to reconnect to send another ping? Also, is the
> > telemetry server rate limited? If so, do you know under which metrics?

Yes, the current server should accept multiple pings over the same connection. If you try it and there are issues, feel free to ni? :RaFromBRC.

There is no rate limiting as far as I know.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note to self: also consider how useful BaseResource is here.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note to self: if we can't re-use the HTTP connection, consider doing the following for pings: "get all pings, upload one, delete one, upload one, delete one..." to avoid reuploading pings and saving user data.

Or if we can re-use, consider sending batches of pings (e.g. upload five, delete five, upload five, delete...).

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We are currently using httpclientandroidlib [1], wrapped in our custom Resource/Delegate system. httpclientandroid lib is an Android port of Apache's HttpClient (which was deprecated for use in Android – they eventually ported their own [2], but we're using this other one).

By default, a single HttpClient instance attempts to re-use connections [3], however, our custom BaseResource/Delegate system does not re-use HttpClients [4]:

    // We could reuse these client instances, except that we mess around
    // with their parameters… so we'd need a pool of some kind.
    client = new DefaultHttpClient(getConnectionManager());

I'll dig in to see how feasible it'd be to unravel the code.

[1]: https://code.google.com/archive/p/httpclientandroidlib/
[2]: https://hc.apache.org/httpcomponents-client-4.3.x/android-port.html
[3]: https://hc.apache.org/httpclient-3.x/performance.html#Connection_persistence

[4]: https://dxr.mozilla.org/mozilla-central/rev/f3f2fa1d7eed5a8262f6401ef18ff8117a3ce43e/mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java#215

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

BaseResource creates a new HttpClient in prepareClient [1], passing in the ConnectionManager it creates. Thus, in order to re-use the TCP connection to re-use the HTTP connection, we need to re-use (I assume) both HttpClient & the ConnectionManager.

Notably, HttpClient has methods closeExpiredConnections & shutdownConnectionManager, meaning we have to worry about when these methods might close some resources we want to keep open. That being said, it looks like the former is never called in a real context and shutdownConnectionManager is only called in tests. We do have to be concerned about the underlying connection manager being closed at different parts of the code, however.

That being said, the comments also back-up that this connectionManager is never closed [2] (...which is also bad). However, the comment could be out-of-date and the code could change in the future.
---

Overall, there is a lot of hard-to-follow behavior here (notably: I'm not familiar with the httpclientclientandroidlib API) – the components are designed to work together in a particular way and it seems like trying to decouple them properly will create a lot of bugs. We could probably hack BaseResource to do what we want but it's likely we might see regressions, either now, or as the code may change in the future using different assumptions.

As such, to get the best performance for telemetry, I think the best course of action is to rewrite this code to use something simpler & more standard (e.g. HttpUrlConnection or a library like OkHttp), but I think that's a work for another bug.

[1]: https://dxr.mozilla.org/mozilla-central/rev/f3f2fa1d7eed5a8262f6401ef18ff8117a3ce43e/mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java#217
[2]: https://dxr.mozilla.org/mozilla-central/rev/f3f2fa1d7eed5a8262f6401ef18ff8117a3ce43e/mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java#268

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The idea of concatenating our JSON strings together and sending all the data as a string didn't occur to me – let's see if this is possible.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #8)
> The idea of concatenating our JSON strings together and sending all the data
> as a string didn't occur to me – let's see if this is possible.

Oh yeah, we can't do this because the url we send the pings to is dependent on the ping being sent. Specifically:
  http://hostname/submit/telemetry/docId/docType/appName/appVersion/appUpdateChannel/appBuildID

The docID will definitely change between pings and the appVersion & appBuildID are also possible to change.