Log connection type to syslogs and to the server.

Log connection type to syslogs and to the server.

Also updated HostStatusObserver to receive transport route information as
part of the OnClientRouteChange() notification, so that logging code
receives connection type in order to log it.

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

[alexeypa (please no reviews), 2012-03-19 22:04:59]

A drive-by review. Please see my comment.

http://codereview.chromium.org/9727005/diff/1/remoting/host/host_event_logger...
File remoting/host/host_event_logger_win.cc (right):

http://codereview.chromium.org/9727005/diff/1/remoting/host/host_event_logger...
remoting/host/host_event_logger_win.cc:101: Log(EVENTLOG_INFORMATION_TYPE,
MSG_HOST_CLIENT_ROUTING_CHANGED, strings);
This requires updating remoting/host/remoting_host_messages.mc file.

Note that normally you would add another message leaving
MSG_HOST_CLIENT_ROUTING_CHANGED intact. This would be needed to make sure that
already logged messages could be shown properly after the host binary is
updated. In this case however, the binary wasn't released yet, so you can patch
the existing message.

[simonmorris, 2012-03-19 22:17:04]

lambroslambrou@, please check that the change to HostEventLoggerPosix won't
adversely affect consumers of the host event logs.

http://codereview.chromium.org/9727005/diff/1/remoting/host/log_to_server.cc
File remoting/host/log_to_server.cc (right):

http://codereview.chromium.org/9727005/diff/1/remoting/host/log_to_server.cc#...
remoting/host/log_to_server.cc:78: LogSessionStateChange(false);
Maybe clear current_connection_type_ here, to protect against a value leaking
from one session to the next?

http://codereview.chromium.org/9727005/diff/1/remoting/host/server_log_entry.h
File remoting/host/server_log_entry.h (right):

http://codereview.chromium.org/9727005/diff/1/remoting/host/server_log_entry....
remoting/host/server_log_entry.h:37: // Adds a filed describing connection type
(direct/stun/relay).
"filed" -> "field".

http://codereview.chromium.org/9727005/diff/1/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/9727005/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:58: std::string GetTypeString() const;
Could this expose a RouteType, instead of a string derived from a RouteType?

The current approach leads to errors if a later CL adds/changes one of the
strings, without making sure that all the consumers of the string will work
properly. Some of the ultimate consumers of the string are a long way from this
code, but nearer to the HostEventLoggers and LogToServer.

[Lambros, 2012-03-19 23:16:29]

On 2012/03/19 22:17:04, simonmorris wrote:
> lambroslambrou@, please check that the change to HostEventLoggerPosix won't
> adversely affect consumers of the host event logs.
That looks fine.

[Sergey Ulanov, 2012-03-19 23:21:03]

http://codereview.chromium.org/9727005/diff/1/remoting/host/host_event_logger...
File remoting/host/host_event_logger_win.cc (right):

http://codereview.chromium.org/9727005/diff/1/remoting/host/host_event_logger...
remoting/host/host_event_logger_win.cc:101: Log(EVENTLOG_INFORMATION_TYPE,
MSG_HOST_CLIENT_ROUTING_CHANGED, strings);
On 2012/03/19 22:04:59, alexeypa wrote:
> This requires updating remoting/host/remoting_host_messages.mc file.

Thanks for noticing it!

 
> Note that normally you would add another message leaving
> MSG_HOST_CLIENT_ROUTING_CHANGED intact. This would be needed to make sure that
> already logged messages could be shown properly after the host binary is
> updated. In this case however, the binary wasn't released yet, so you can
patch
> the existing message.

Whoa, that sucks.

http://codereview.chromium.org/9727005/diff/1/remoting/host/log_to_server.cc
File remoting/host/log_to_server.cc (right):

http://codereview.chromium.org/9727005/diff/1/remoting/host/log_to_server.cc#...
remoting/host/log_to_server.cc:78: LogSessionStateChange(false);
On 2012/03/19 22:17:05, simonmorris wrote:
> Maybe clear current_connection_type_ here, to protect against a value leaking
> from one session to the next?

Added a separate flag that indicates that the type is set and DCHECK in
LogSessionStateChange().

http://codereview.chromium.org/9727005/diff/1/remoting/host/server_log_entry.h
File remoting/host/server_log_entry.h (right):

http://codereview.chromium.org/9727005/diff/1/remoting/host/server_log_entry....
remoting/host/server_log_entry.h:37: // Adds a filed describing connection type
(direct/stun/relay).
On 2012/03/19 22:17:05, simonmorris wrote:
> "filed" -> "field".

Done.

http://codereview.chromium.org/9727005/diff/1/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/9727005/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:58: std::string GetTypeString() const;
On 2012/03/19 22:17:05, simonmorris wrote:
> Could this expose a RouteType, instead of a string derived from a RouteType?
> 
> The current approach leads to errors if a later CL adds/changes one of the
> strings, without making sure that all the consumers of the string will work
> properly. Some of the ultimate consumers of the string are a long way from
this
> code, but nearer to the HostEventLoggers and LogToServer.

This is a struct and the type field is public. This is just a helper method to
get string representation of the type.

I've changed this method to static, and pass RouteType instead of a string to
AddConnectionTypeField(), but it still uses this function to convert the value -
otherwise I would have to duplicate this code.

[simonmorris, 2012-03-19 23:31:42]

lgtm

On 2012/03/19 23:21:03, sergeyu wrote:
>
http://codereview.chromium.org/9727005/diff/1/remoting/host/host_event_logger...
> File remoting/host/host_event_logger_win.cc (right):
> 
>
http://codereview.chromium.org/9727005/diff/1/remoting/host/host_event_logger...
> remoting/host/host_event_logger_win.cc:101: Log(EVENTLOG_INFORMATION_TYPE,
> MSG_HOST_CLIENT_ROUTING_CHANGED, strings);
> On 2012/03/19 22:04:59, alexeypa wrote:
> > This requires updating remoting/host/remoting_host_messages.mc file.
> 
> Thanks for noticing it!
> 
>  
> > Note that normally you would add another message leaving
> > MSG_HOST_CLIENT_ROUTING_CHANGED intact. This would be needed to make sure
that
> > already logged messages could be shown properly after the host binary is
> > updated. In this case however, the binary wasn't released yet, so you can
> patch
> > the existing message.
> 
> Whoa, that sucks.
> 
> http://codereview.chromium.org/9727005/diff/1/remoting/host/log_to_server.cc
> File remoting/host/log_to_server.cc (right):
> 
>
http://codereview.chromium.org/9727005/diff/1/remoting/host/log_to_server.cc#...
> remoting/host/log_to_server.cc:78: LogSessionStateChange(false);
> On 2012/03/19 22:17:05, simonmorris wrote:
> > Maybe clear current_connection_type_ here, to protect against a value
leaking
> > from one session to the next?
> 
> Added a separate flag that indicates that the type is set and DCHECK in
> LogSessionStateChange().
> 
> http://codereview.chromium.org/9727005/diff/1/remoting/host/server_log_entry.h
> File remoting/host/server_log_entry.h (right):
> 
>
http://codereview.chromium.org/9727005/diff/1/remoting/host/server_log_entry....
> remoting/host/server_log_entry.h:37: // Adds a filed describing connection
type
> (direct/stun/relay).
> On 2012/03/19 22:17:05, simonmorris wrote:
> > "filed" -> "field".
> 
> Done.
> 
> http://codereview.chromium.org/9727005/diff/1/remoting/protocol/transport.h
> File remoting/protocol/transport.h (right):
> 
>
http://codereview.chromium.org/9727005/diff/1/remoting/protocol/transport.h#n...
> remoting/protocol/transport.h:58: std::string GetTypeString() const;
> On 2012/03/19 22:17:05, simonmorris wrote:
> > Could this expose a RouteType, instead of a string derived from a RouteType?
> > 
> > The current approach leads to errors if a later CL adds/changes one of the
> > strings, without making sure that all the consumers of the string will work
> > properly. Some of the ultimate consumers of the string are a long way from
> this
> > code, but nearer to the HostEventLoggers and LogToServer.
> 
> This is a struct and the type field is public. This is just a helper method to
> get string representation of the type.
> 
> I've changed this method to static, and pass RouteType instead of a string to
> AddConnectionTypeField(), but it still uses this function to convert the value
-
> otherwise I would have to duplicate this code.

[commit-bot: I haz the power, 2012-03-19 23:41:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9727005/7001

[commit-bot: I haz the power, 2012-03-20 00:34:00]

Try job failure for 9727005-7001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-20 18:33:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9727005/12001

[commit-bot: I haz the power, 2012-03-20 18:34:18]

No file was found in this patchset.

[commit-bot: I haz the power, 2012-03-20 18:35:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9727005/16001

[commit-bot: I haz the power, 2012-03-20 19:47:23]

Try job failure for 9727005-16001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-20 19:54:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9727005/22001

[commit-bot: I haz the power, 2012-03-20 21:10:29]

Try job failure for 9727005-22001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-20 21:16:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9727005/31001

[commit-bot: I haz the power, 2012-03-20 22:20:40]

Try job failure for 9727005-31001 (retry) on mac_rel for step "unit_tests"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...