Implement stats reporting in Android client

Implement stats reporting in Android client

This reports the same connection statistics and session change events
that are currently reported by the web-app.

Caveats:
This does not provide Chrome or Webapp versions - instead it
reports the Android OS version, and the app version, under
differently-named keys.
Session termination events don't seem to be reported. This is a
limitation of the Android client that might not be easy to work around.

BUG=368015
R=rmsousa@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269404

[Lambros, 2014-05-07 18:43:58]

There is some code duplication with the LogToServer and ServerLogEntry classes
in remoting/host. Here I've appended 'Client' to the class names, so they don't
clash. Feel free to suggest better names. I propose to do the refactoring after
the M36 cut.

Also, this version is broken - I'm getting an error from libjingle that the
stanzas contain unsupported elements. Here are some example XML stanzas being
sent by this CL versus the webapp:

Sent by Android:

<rem:log id="0" xmlns:rem="google:remoting"><rem:entry app-version="36.0.1968.0"
cpu="armv7l" event-name="session-state" mode="me2me" os-name="Android"
os-version="4.4.3" role="client" session-id="atYs2pTo6TRY7Kal3A3q"
session-state="connecting"/></rem:log>

Sent by webapp:

"<cli:iq to="remoting@bot.talk.google.com" type="set"
xmlns:cli="jabber:client"><gr:log xmlns:gr="google:remoting"><gr:entry
role="client" event-name="session-state" session-state="connecting" mode="me2me"
os-name="Linux" cpu="x86_64" browser-version="35.0.1916.69"
webapp-version="36.0.1967.0" session-id="dgCPIytg0wB9QNE2XJCb"
/></gr:log></cli:iq>"

[rmsousa, 2014-05-07 19:14:10]

Please just copy server_log_entry and log_to_server, and only change where
needed (rather than rewriting those files). That will make it much easier to
review the diffs (and in the future, much easier to refactor). If we know we
will integrate them just after the branch, you could just leave a comment to
that effect and even leave the names as-is - they shouldn't clash, as client/
and host/ should never be linked into the same binary.

For the failure, the problem is that you're not using IqSender (see
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/log_...),
and instead are trying to send the raw stanza directly (which will be missing
the top level cli:iq tag). Please use IqSender, unless there's a good reason not
to.

On 2014/05/07 18:43:58, Lambros wrote:
> There is some code duplication with the LogToServer and ServerLogEntry classes
> in remoting/host. Here I've appended 'Client' to the class names, so they
don't
> clash. Feel free to suggest better names. I propose to do the refactoring
after
> the M36 cut.
> 
> Also, this version is broken - I'm getting an error from libjingle that the
> stanzas contain unsupported elements. Here are some example XML stanzas being
> sent by this CL versus the webapp:
> 
> Sent by Android:
> 
> <rem:log id="0" xmlns:rem="google:remoting"><rem:entry
app-version="36.0.1968.0"
> cpu="armv7l" event-name="session-state" mode="me2me" os-name="Android"
> os-version="4.4.3" role="client" session-id="atYs2pTo6TRY7Kal3A3q"
> session-state="connecting"/></rem:log>
> 
> Sent by webapp:
> 
> "<cli:iq mailto:to="remoting@bot.talk.google.com" type="set"
> xmlns:cli="jabber:client"><gr:log xmlns:gr="google:remoting"><gr:entry
> role="client" event-name="session-state" session-state="connecting"
mode="me2me"
> os-name="Linux" cpu="x86_64" browser-version="35.0.1916.69"
> webapp-version="36.0.1967.0" session-id="dgCPIytg0wB9QNE2XJCb"
> /></gr:log></cli:iq>"

[Lambros, 2014-05-08 02:03:48]

PTAL. Diff between #2 and #3 will show what changes I've made to the
remoting/host versions of these classes.

[rmsousa, 2014-05-09 00:38:47]

lgtm

[Lambros, 2014-05-09 02:45:57]

The CQ bit was checked by lambroslambrou@chromium.org

[commit-bot: I haz the power, 2014-05-09 02:47:23]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/270613005/...

[Lambros, 2014-05-09 19:21:39]

The CQ bit was unchecked by lambroslambrou@chromium.org

[Lambros, 2014-05-09 20:49:06]

Committed patchset #3 manually as r269404 (presubmit successful).

[Sergey Ulanov, 2014-05-16 22:44:18]

https://codereview.chromium.org/270613005/diff/60001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/270613005/diff/60001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:341:
"remoting@bot.talk.google.com"));
Move kDirectoryBotJid from remoting/host/service_urls.cc to
remoting/base/constants.h?

https://codereview.chromium.org/270613005/diff/60001/remoting/client/log_to_s...
File remoting/client/log_to_server.cc (right):

https://codereview.chromium.org/270613005/diff/60001/remoting/client/log_to_s...
remoting/client/log_to_server.cc:139: if (iq_sender_ == NULL) {
nit: !iq_sender_

https://codereview.chromium.org/270613005/diff/60001/remoting/client/log_to_s...
remoting/client/log_to_server.cc:142: if (pending_entries_.empty()) {
nit: Can combine these two ifs into one.

https://codereview.chromium.org/270613005/diff/60001/remoting/client/log_to_s...
remoting/client/log_to_server.cc:153: scoped_ptr<IqRequest> req =
iq_sender_->SendIq(
|req| is not used, so you don't need it. Just call SendId() and ignore the
result

https://codereview.chromium.org/270613005/diff/60001/remoting/client/log_to_s...
remoting/client/log_to_server.cc:157: return;
Don't need return at the end of a function

[Sergey Ulanov, 2014-05-16 22:52:55]

On 2014/05/07 19:14:10, rmsousa wrote:
> For the failure, the problem is that you're not using IqSender (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/log_...),
> and instead are trying to send the raw stanza directly (which will be missing
> the top level cli:iq tag). Please use IqSender, unless there's a good reason
not
> to.
 
IqSender is useful only when you are expecting response. In this case we are not
expecting the response so there is no reason to use it instead of
SignalStrategy::SendStanza(), that's why I told Lambros to use SignalStrategy
directly.
Lambros, as I mentioned to you when we talked about it,
SignalStrategy::GetNextId() should be used to create and assign stanza ID when
using SendStanza(), but I don't mind using IqSender as in the change you landed.

[Sergey Ulanov, 2014-05-16 22:56:57]

https://codereview.chromium.org/270613005/diff/60001/remoting/client/log_to_s...
File remoting/client/log_to_server.cc (right):

https://codereview.chromium.org/270613005/diff/60001/remoting/client/log_to_s...
remoting/client/log_to_server.cc:121: iq_sender_.reset(new
IqSender(signal_strategy_));
It's not really necessary. IqSender can be created only once in the constructor,
and then it should be able to handle the case when signaling is down (by
returning NULL from SendIq()).