Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2856243002:
[Chromoting] Add ProcessStatsDispatcher to send process stats to the client
Description[Chromoting] Add ProcessStatsDispatcher to send process stats to the client
This change starts a new channel (proc_stats) to dedicate sending process
resource usage. A new function ConnectionToClient::stats_stub() is also added:
WebRtcConnectionToClient returns ProcessStatsDispatcher, other implementations
returns nullptr or FakeProcessStatsDispatcher.
Refer to pending change https://codereview.chromium.org/2858813002/ for the
usage.
BUG=650926
Patch Set 1 #Patch Set 2 : Change ProcessStatsDispatcher into ProcessStatsAgent and send the message through ClientStub #
Total comments: 14
Patch Set 3 : Partially resolve the review comments #
Messages
Total messages: 44 (33 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Chromoting] Add ProcessStatsDispatcher to send process stats to the client This change starts a new channel (proc_stats) to dedicate sending process resource usage. A new function ConnectionToClient::stats_stub() is also added: WebRtcConnectionToClient returns ProcessStatsDispatcher, other implementations returns nullptr or FakeProcessStatsDispatcher. (TODO until change https://codereview.chromium.org/2860803002/.) Refer to pending change https://codereview.chromium.org/2858813002/ for the usage. BUG=650926 ========== to ========== [Chromoting] Add ProcessStatsDispatcher to send process stats to the client This change starts a new channel (proc_stats) to dedicate sending process resource usage. A new function ConnectionToClient::stats_stub() is also added: WebRtcConnectionToClient returns ProcessStatsDispatcher, other implementations returns nullptr or FakeProcessStatsDispatcher. Refer to pending change https://codereview.chromium.org/2858813002/ for the usage. BUG=650926 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org, joedow@chromium.org
I made a comment in the other CL, but I'm not sure process stats warrants a new data channel. The amount of data should be small and we can send it right in ClientSession. If we do decide to use a WebRTC channel, then I think we should do some work on a scalable design as adding methods for each new channel seems like it will clutter our class interfaces before too long.
On 2017/05/03 21:43:19, joedow wrote: > I made a comment in the other CL, but I'm not sure process stats warrants a new > data channel. > > The amount of data should be small and we can send it right in ClientSession. > > If we do decide to use a WebRTC channel, then I think we should do some work on > a scalable design as adding methods for each new channel seems like it will > clutter our class interfaces before too long. Yes, I read the comment in https://codereview.chromium.org/2860803002/. I think event channel is for input events only. We also have dedicate channels (video_stats:*) for video stats. So I think it should be fine to create a new channel for process statistics. If my understanding is correct, creating a channel is cheap, it's just a logic router in both client and server side.
The host to client channel is used for many types of messages (for instance we use it for SK message forwarding). Let's talk offline about the design.
On 2017/05/08 16:12:54, joedow wrote: > The host to client channel is used for many types of messages (for instance we > use it for SK message forwarding). > > Let's talk offline about the design. I think you must mean ExtensionMessage (https://cs.chromium.org/chromium/src/remoting/proto/control.proto?rcl=c7b7263...) and SecurityKeyExtensionSession (https://cs.chromium.org/chromium/src/remoting/host/security_key/security_key_...). I have also considered to use it. But then I found VideoStats channels. Meanwhile, the disadvantage of using ExtensionMessage is that we need to manually encode and decode the AggregatedProcessResourceUsage.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
Sergey, would you mind to have a look at this change?
This is much cleaner, thanks for the update! I just have a few comments/questions and then I am good with this. https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants.cc File remoting/base/constants.cc (right): https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants... remoting/base/constants.cc:22: const char kSecurityKeyType[] = "gnubby-auth"; nit, you could remove the namespace (like the channel names above) and include the NS in the name: kSecurityKeyExtensionMessage This would make the ChannelName and MessageName vars use a similar format. https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants.h File remoting/base/constants.h (right): https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants... remoting/base/constants.h:19: extern const char kProcessStatsChannelName[]; I don't see this being used in the current iteration, are you planning on using it in a later CL or can it be removed. https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants... remoting/base/constants.h:29: extern const char kSecurityKeyType[]; This is a nice addition, thanks! https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... File remoting/protocol/process_stats_adapter.cc (right): https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... remoting/protocol/process_stats_adapter.cc:29: CHECK(usage.SerializeToString(&data)); Should this break a release build? I think a DCHECK is fine, but if we released a host where there was a problem sending stats data, I don't think I would want the host to be unusable.
The CL description doesn't seem to match the content. Can you please update the description. There are 3 ways this data can be send from host to client: 1. In a separate channel. I initially assumed that was the plan. In this case the messages would be serialized as protobufs. 2. Over control channel. Messages serialized as protobufs. In that case you'd want to add a new method in ClientStub and a new field in ControlMessage for the new message type. 3. Extension messages. In this case the messages are normally serialized using JSON then folded into ExtensionMessage protobuf. Extension messages were added as a way to extend protocol from JS layer in the chrome webapp. Normally these extensions should be implemented using HostExtension and HostExtensionSession. It appears you are trying to piggyback on ExtensionMessage here without implementing a host extension, which doesn't look right. I would recommend (1), but (2) would work as well. With WebRTC data channels are cheap and there are no downsides creating a new data channel here. https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants.cc File remoting/base/constants.cc (right): https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants... remoting/base/constants.cc:22: const char kSecurityKeyType[] = "gnubby-auth"; On 2017/05/26 16:14:10, joedow wrote: > nit, you could remove the namespace (like the channel names above) and include > the NS in the name: kSecurityKeyExtensionMessage > > This would make the ChannelName and MessageName vars use a similar format. +1 https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants... remoting/base/constants.cc:25: } // namespace extension_message https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... File remoting/protocol/process_stats_adapter.cc (right): https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... remoting/protocol/process_stats_adapter.cc:29: CHECK(usage.SerializeToString(&data)); All extension messages are serialized using JSON. Using protobuf here would make it inconsistent. https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... remoting/protocol/process_stats_adapter.cc:32: client_stub_->DeliverHostMessage(message); If we are using protocol extensions to send these messages then it should be implemented using HostExtension & HostExtensionSession interfaces. https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... File remoting/protocol/process_stats_adapter.h (right): https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... remoting/protocol/process_stats_adapter.h:26: private: nit: add empty line here
On 2017/05/26 19:27:30, Sergey Ulanov wrote: > The CL description doesn't seem to match the content. Can you please update the > description. > > There are 3 ways this data can be send from host to client: > 1. In a separate channel. I initially assumed that was the plan. In this case > the messages would be serialized as protobufs. > 2. Over control channel. Messages serialized as protobufs. In that case you'd > want to add a new method in ClientStub and a new field in ControlMessage for the > new message type. > 3. Extension messages. In this case the messages are normally serialized using > JSON then folded into ExtensionMessage protobuf. Extension messages were added > as a way to extend protocol from JS layer in the chrome webapp. Normally these > extensions should be implemented using HostExtension and HostExtensionSession. > It appears you are trying to piggyback on ExtensionMessage here without > implementing a host extension, which doesn't look right. > > I would recommend (1), but (2) would work as well. With WebRTC data channels are > cheap and there are no downsides creating a new data channel here. > > https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants.cc > File remoting/base/constants.cc (right): > > https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants... > remoting/base/constants.cc:22: const char kSecurityKeyType[] = "gnubby-auth"; > On 2017/05/26 16:14:10, joedow wrote: > > nit, you could remove the namespace (like the channel names above) and include > > the NS in the name: kSecurityKeyExtensionMessage > > > > This would make the ChannelName and MessageName vars use a similar format. > > +1 > > https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants... > remoting/base/constants.cc:25: } > // namespace extension_message > > https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... > File remoting/protocol/process_stats_adapter.cc (right): > > https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... > remoting/protocol/process_stats_adapter.cc:29: > CHECK(usage.SerializeToString(&data)); > All extension messages are serialized using JSON. Using protobuf here would make > it inconsistent. > > https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... > remoting/protocol/process_stats_adapter.cc:32: > client_stub_->DeliverHostMessage(message); > If we are using protocol extensions to send these messages then it should be > implemented using HostExtension & HostExtensionSession interfaces. > > https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... > File remoting/protocol/process_stats_adapter.h (right): > > https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... > remoting/protocol/process_stats_adapter.h:26: private: > nit: add empty line here Yes, the first iteration uses the method (1). I have the same feeling that a WebRTC data channel is cheap. If we agree on this, I can revert it to the first iteration.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
I don't think a revert to the first iteration makes sense. I have no problem with using a WebRTC channel, my concern is that there isn't a clean way to expose that functionality outside of the Connection classes (this is something Zijie and I also discussed offline). I don't like having to update multiple interfaces each time we decide to create a P2P channel for a new feature. Perhaps a quick whiteboard session would be good here.
https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants.cc File remoting/base/constants.cc (right): https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants... remoting/base/constants.cc:22: const char kSecurityKeyType[] = "gnubby-auth"; On 2017/05/26 16:14:10, joedow wrote: > nit, you could remove the namespace (like the channel names above) and include > the NS in the name: kSecurityKeyExtensionMessage > > This would make the ChannelName and MessageName vars use a similar format. Done. https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants.h File remoting/base/constants.h (right): https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants... remoting/base/constants.h:19: extern const char kProcessStatsChannelName[]; On 2017/05/26 16:14:10, joedow wrote: > I don't see this being used in the current iteration, are you planning on using > it in a later CL or can it be removed. I forgot to remove it. https://codereview.chromium.org/2856243002/diff/60001/remoting/base/constants... remoting/base/constants.h:29: extern const char kSecurityKeyType[]; On 2017/05/26 16:14:10, joedow wrote: > This is a nice addition, thanks! :) https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... File remoting/protocol/process_stats_adapter.cc (right): https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... remoting/protocol/process_stats_adapter.cc:29: CHECK(usage.SerializeToString(&data)); On 2017/05/26 16:14:10, joedow wrote: > Should this break a release build? I think a DCHECK is fine, but if we released > a host where there was a problem sending stats data, I don't think I would want > the host to be unusable. Reasonable. Though I think there is zero reason this function call would fail. (The only possible failure here is a 2G+ message https://cs.chromium.org/chromium/src/third_party/protobuf/src/google/protobuf...) https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... File remoting/protocol/process_stats_adapter.h (right): https://codereview.chromium.org/2856243002/diff/60001/remoting/protocol/proce... remoting/protocol/process_stats_adapter.h:26: private: On 2017/05/26 19:27:30, Sergey Ulanov wrote: > nit: add empty line here Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zijiehe@chromium.org changed reviewers: - jamiewalch@chromium.org, joedow@chromium.org, sergeyu@chromium.org |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
