You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Dominic Hamon <dh...@twopensource.com> on 2014/03/14 17:31:48 UTC
Review Request 19208: Move HTTP request logging to ProcessBase and add peer
IP address.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19208/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ian Downes.
Bugs: MESOS-1096
https://issues.apache.org/jira/browse/MESOS-1096
Repository: mesos-git
Description
-------
see summary
Diffs
-----
3rdparty/libprocess/src/process.cpp 6c6acc03ad78779e8f25b1a4584069fd377f647b
src/master/http.cpp a019f15615d028f3d3628b2611709fc8a3a81bf8
src/slave/http.cpp 594032da1b2edd47961cd8201acd093b22fa30ae
Diff: https://reviews.apache.org/r/19208/diff/
Testing
-------
make check
ran locally and connected to :5050 endpoint and watched log
Thanks,
Dominic Hamon
Re: Review Request 19208: Move HTTP request logging to ProcessBase and add
peer IP address.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19208/
-----------------------------------------------------------
(Updated March 19, 2014, 3:49 p.m.)
Review request for mesos, Benjamin Hindman and Ian Downes.
Changes
-------
split out mesos changes.
Bugs: MESOS-1096
https://issues.apache.org/jira/browse/MESOS-1096
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am df149ddc8669b0c541361922631f4f3958bd096d
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 47fe92c380de3e2abc625dc936afbd034280b76a
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp ba8463cc152701d6eb1deba8d1561688ecf5b7b5
3rdparty/libprocess/include/process/future.hpp 5e124760ca304082da506c91b6341e784f103d93
3rdparty/libprocess/include/process/socket.hpp dbcb4f4c2eb12663158057a844b4511d6dde0508
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/process.cpp 6c6acc03ad78779e8f25b1a4584069fd377f647b
bin/mesos-slave-flags.sh.in 8c936aa06e994a87a8b09b31c907868bf9be38c7
configure.ac 9a6de87fa6523bf8137f5d74ea0b7c6cbd947d3a
docs/configuration.md 2dbe0f81aea0e0db41f3eb4b0554ae5e9f13da2e
docs/getting-started.md d7ced76125c6aba0d7e1a0f4ca8165652b17af0d
src/Makefile.am 0775a0df293e945d41c7ba90fd1bbb503ae22f9e
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 668647fdfc0e203fcde59263256659ba14e29960
src/linux/cgroups.hpp 6056e3ae59ed813d75b1bb46b276872530155ec7
src/linux/cgroups.cpp eff15af417f3827b4a1f73770b98dc9fccbc397e
src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
src/local/local.cpp 2cfdf49c9eb92302502eb50c623f9606977b88b6
src/log/log.cpp d9b26871bfa1b3c36f5e7d879eb7b67132af1b33
src/master/detector.cpp 8b10061de12a35ad6624db594dc4ec36502b2420
src/master/flags.hpp 024f86d93824a20ce42c28b8264576f1cb715d0e
src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
src/master/main.cpp f12f20a1eabd163c4f35056bf01f28f3edd408a9
src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
src/sched/sched.cpp 3684cfe9d153cab5f4ea86094fffd814ce74baa1
src/slave/containerizer/containerizer.cpp faf3d0cf97aafd88eab82ae39e2255b1eb4ce294
src/slave/containerizer/isolators/cgroups/cpushare.cpp 11665dbe88e3920fd9d8a2259987611d49e85462
src/slave/containerizer/mesos_containerizer.cpp c819c97d96d232a1de3c1ed2fc848bebf66981f7
src/slave/slave.cpp d8d3e0fa54972201d72b2650ec0ba922a4912d54
src/slave/status_update_manager.cpp 5d5cf234ef2dd47fa4b1f67be761dbca31659451
src/tests/cluster.hpp 40d9f8c18307aead2374396710f9a82466e3a716
src/tests/environment.cpp feeca042265eede40e87db3ee5ab89be04509a90
src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
src/tests/state_tests.cpp d0e084070c566ee7d751a8e1279772e05b966145
Diff: https://reviews.apache.org/r/19208/diff/
Testing
-------
make check
ran locally and connected to :5050 endpoint and watched log
Thanks,
Dominic Hamon
Re: Review Request 19208: Move HTTP request logging to ProcessBase and add
peer IP address.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19208/#review37357
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19208]
All tests passed.
- Mesos ReviewBot
On March 14, 2014, 4:31 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19208/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 4:31 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-1096
> https://issues.apache.org/jira/browse/MESOS-1096
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 6c6acc03ad78779e8f25b1a4584069fd377f647b
> src/master/http.cpp a019f15615d028f3d3628b2611709fc8a3a81bf8
> src/slave/http.cpp 594032da1b2edd47961cd8201acd093b22fa30ae
>
> Diff: https://reviews.apache.org/r/19208/diff/
>
>
> Testing
> -------
>
> make check
>
> ran locally and connected to :5050 endpoint and watched log
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19208: Move HTTP request logging to ProcessBase and add
peer IP address.
Posted by Ben Mahler <be...@gmail.com>.
> On March 14, 2014, 5:23 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 3207-3208
> > <https://reviews.apache.org/r/19208/diff/1/?file=519368#file519368line3207>
> >
> > We avoid doing LOG(INFO) in our 3rd party libraries. Convert this to VLOG instead.
> >
> > More importantly, at Twitter we have observed that these LOGs are actually useful during debugging and hence we wanted these to be always present (i.e., LOG(INFO) instead of VLOG). That's likely the reason these are in http.cpp files.
> >
>
> Dominic Hamon wrote:
> This doesn't add any more log-spam than we already had as we were logging this anyway, albeit without the IP address. We can't easily expose the IP address down to the http files (and probably shouldn't) whereas this is really useful debugging information.
>
> This should be a LOG(INFO), or at least a log to a separate HTTP access log file if the main log is too sensitive to it.
>
> Vinod Kone wrote:
> Rebase?
>
> More importantly, this is not an issue of log spam. We have deliberately avoided adding LOG(INFO) statements in stout and libprocess (the only violation i see is in the profiler but that should be fixed).
>
> ? ack "LOG\(INFO" ../3rdparty/libprocess/
> ../3rdparty/libprocess/include/process/profiler.hpp
> 58: LOG(INFO) << "Starting Profiler";
> 94: LOG(INFO) << "Stopping Profiler";
> ? ack "LOG\(INFO" ../3rdparty/libprocess/3rdparty/stout
> ? ?
>
>
> How about adding the sender ip/hostname as a member in "http::Request"? Not sure if thats the right place. BenM?
>
>
> Dominic Hamon wrote:
> Adding sender IP/hostname to http::Request sounds reasonable, though it does mean that every endpoint is responsible for logging the info. For something so important to debugging, that seems fragile.
>
> Is the concern about having LOG(INFO) in libprocess that it will get out of hand?
>
> Every HTTP server I know of has some kind of default access logging, precisely because it is vital information. Moving it to a custom log is doable, but requires infrastructure around log rotation that may be a pain to deal with.
>
> Ben Mahler wrote:
> I like Vinod's suggestion of adding this information into the http::Request and logging that information in our existing Mesos endpoints, that way we can have an immediate improvement without getting overly ambitious here. :)
On second thought, this breaks http::Request a bit in that we would be including information that is not present in the underlying HTTP request, so I'm a bit hesitant to do that.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19208/#review37220
-----------------------------------------------------------
On March 19, 2014, 10:49 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19208/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 10:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-1096
> https://issues.apache.org/jira/browse/MESOS-1096
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am df149ddc8669b0c541361922631f4f3958bd096d
> 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 47fe92c380de3e2abc625dc936afbd034280b76a
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp ba8463cc152701d6eb1deba8d1561688ecf5b7b5
> 3rdparty/libprocess/include/process/future.hpp 5e124760ca304082da506c91b6341e784f103d93
> 3rdparty/libprocess/include/process/socket.hpp dbcb4f4c2eb12663158057a844b4511d6dde0508
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/process.cpp 6c6acc03ad78779e8f25b1a4584069fd377f647b
> bin/mesos-slave-flags.sh.in 8c936aa06e994a87a8b09b31c907868bf9be38c7
> configure.ac 9a6de87fa6523bf8137f5d74ea0b7c6cbd947d3a
> docs/configuration.md 2dbe0f81aea0e0db41f3eb4b0554ae5e9f13da2e
> docs/getting-started.md d7ced76125c6aba0d7e1a0f4ca8165652b17af0d
> src/Makefile.am 0775a0df293e945d41c7ba90fd1bbb503ae22f9e
> src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 668647fdfc0e203fcde59263256659ba14e29960
> src/linux/cgroups.hpp 6056e3ae59ed813d75b1bb46b276872530155ec7
> src/linux/cgroups.cpp eff15af417f3827b4a1f73770b98dc9fccbc397e
> src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
> src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
> src/local/local.cpp 2cfdf49c9eb92302502eb50c623f9606977b88b6
> src/log/log.cpp d9b26871bfa1b3c36f5e7d879eb7b67132af1b33
> src/master/detector.cpp 8b10061de12a35ad6624db594dc4ec36502b2420
> src/master/flags.hpp 024f86d93824a20ce42c28b8264576f1cb715d0e
> src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
> src/master/main.cpp f12f20a1eabd163c4f35056bf01f28f3edd408a9
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/sched/sched.cpp 3684cfe9d153cab5f4ea86094fffd814ce74baa1
> src/slave/containerizer/containerizer.cpp faf3d0cf97aafd88eab82ae39e2255b1eb4ce294
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 11665dbe88e3920fd9d8a2259987611d49e85462
> src/slave/containerizer/mesos_containerizer.cpp c819c97d96d232a1de3c1ed2fc848bebf66981f7
> src/slave/slave.cpp d8d3e0fa54972201d72b2650ec0ba922a4912d54
> src/slave/status_update_manager.cpp 5d5cf234ef2dd47fa4b1f67be761dbca31659451
> src/tests/cluster.hpp 40d9f8c18307aead2374396710f9a82466e3a716
> src/tests/environment.cpp feeca042265eede40e87db3ee5ab89be04509a90
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
> src/tests/state_tests.cpp d0e084070c566ee7d751a8e1279772e05b966145
>
> Diff: https://reviews.apache.org/r/19208/diff/
>
>
> Testing
> -------
>
> make check
>
> ran locally and connected to :5050 endpoint and watched log
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19208: Move HTTP request logging to ProcessBase and add
peer IP address.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On March 14, 2014, 10:23 a.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 3207-3208
> > <https://reviews.apache.org/r/19208/diff/1/?file=519368#file519368line3207>
> >
> > We avoid doing LOG(INFO) in our 3rd party libraries. Convert this to VLOG instead.
> >
> > More importantly, at Twitter we have observed that these LOGs are actually useful during debugging and hence we wanted these to be always present (i.e., LOG(INFO) instead of VLOG). That's likely the reason these are in http.cpp files.
> >
This doesn't add any more log-spam than we already had as we were logging this anyway, albeit without the IP address. We can't easily expose the IP address down to the http files (and probably shouldn't) whereas this is really useful debugging information.
This should be a LOG(INFO), or at least a log to a separate HTTP access log file if the main log is too sensitive to it.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19208/#review37220
-----------------------------------------------------------
On March 19, 2014, 3:49 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19208/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 3:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-1096
> https://issues.apache.org/jira/browse/MESOS-1096
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am df149ddc8669b0c541361922631f4f3958bd096d
> 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 47fe92c380de3e2abc625dc936afbd034280b76a
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp ba8463cc152701d6eb1deba8d1561688ecf5b7b5
> 3rdparty/libprocess/include/process/future.hpp 5e124760ca304082da506c91b6341e784f103d93
> 3rdparty/libprocess/include/process/socket.hpp dbcb4f4c2eb12663158057a844b4511d6dde0508
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/process.cpp 6c6acc03ad78779e8f25b1a4584069fd377f647b
> bin/mesos-slave-flags.sh.in 8c936aa06e994a87a8b09b31c907868bf9be38c7
> configure.ac 9a6de87fa6523bf8137f5d74ea0b7c6cbd947d3a
> docs/configuration.md 2dbe0f81aea0e0db41f3eb4b0554ae5e9f13da2e
> docs/getting-started.md d7ced76125c6aba0d7e1a0f4ca8165652b17af0d
> src/Makefile.am 0775a0df293e945d41c7ba90fd1bbb503ae22f9e
> src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 668647fdfc0e203fcde59263256659ba14e29960
> src/linux/cgroups.hpp 6056e3ae59ed813d75b1bb46b276872530155ec7
> src/linux/cgroups.cpp eff15af417f3827b4a1f73770b98dc9fccbc397e
> src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
> src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
> src/local/local.cpp 2cfdf49c9eb92302502eb50c623f9606977b88b6
> src/log/log.cpp d9b26871bfa1b3c36f5e7d879eb7b67132af1b33
> src/master/detector.cpp 8b10061de12a35ad6624db594dc4ec36502b2420
> src/master/flags.hpp 024f86d93824a20ce42c28b8264576f1cb715d0e
> src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
> src/master/main.cpp f12f20a1eabd163c4f35056bf01f28f3edd408a9
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/sched/sched.cpp 3684cfe9d153cab5f4ea86094fffd814ce74baa1
> src/slave/containerizer/containerizer.cpp faf3d0cf97aafd88eab82ae39e2255b1eb4ce294
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 11665dbe88e3920fd9d8a2259987611d49e85462
> src/slave/containerizer/mesos_containerizer.cpp c819c97d96d232a1de3c1ed2fc848bebf66981f7
> src/slave/slave.cpp d8d3e0fa54972201d72b2650ec0ba922a4912d54
> src/slave/status_update_manager.cpp 5d5cf234ef2dd47fa4b1f67be761dbca31659451
> src/tests/cluster.hpp 40d9f8c18307aead2374396710f9a82466e3a716
> src/tests/environment.cpp feeca042265eede40e87db3ee5ab89be04509a90
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
> src/tests/state_tests.cpp d0e084070c566ee7d751a8e1279772e05b966145
>
> Diff: https://reviews.apache.org/r/19208/diff/
>
>
> Testing
> -------
>
> make check
>
> ran locally and connected to :5050 endpoint and watched log
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19208: Move HTTP request logging to ProcessBase and add
peer IP address.
Posted by Vinod Kone <vi...@gmail.com>.
> On March 14, 2014, 5:23 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 3207-3208
> > <https://reviews.apache.org/r/19208/diff/1/?file=519368#file519368line3207>
> >
> > We avoid doing LOG(INFO) in our 3rd party libraries. Convert this to VLOG instead.
> >
> > More importantly, at Twitter we have observed that these LOGs are actually useful during debugging and hence we wanted these to be always present (i.e., LOG(INFO) instead of VLOG). That's likely the reason these are in http.cpp files.
> >
>
> Dominic Hamon wrote:
> This doesn't add any more log-spam than we already had as we were logging this anyway, albeit without the IP address. We can't easily expose the IP address down to the http files (and probably shouldn't) whereas this is really useful debugging information.
>
> This should be a LOG(INFO), or at least a log to a separate HTTP access log file if the main log is too sensitive to it.
Rebase?
More importantly, this is not an issue of log spam. We have deliberately avoided adding LOG(INFO) statements in stout and libprocess (the only violation i see is in the profiler but that should be fixed).
? ack "LOG\(INFO" ../3rdparty/libprocess/
../3rdparty/libprocess/include/process/profiler.hpp
58: LOG(INFO) << "Starting Profiler";
94: LOG(INFO) << "Stopping Profiler";
? ack "LOG\(INFO" ../3rdparty/libprocess/3rdparty/stout
? ?
How about adding the sender ip/hostname as a member in "http::Request"? Not sure if thats the right place. BenM?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19208/#review37220
-----------------------------------------------------------
On March 19, 2014, 10:49 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19208/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 10:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-1096
> https://issues.apache.org/jira/browse/MESOS-1096
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am df149ddc8669b0c541361922631f4f3958bd096d
> 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 47fe92c380de3e2abc625dc936afbd034280b76a
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp ba8463cc152701d6eb1deba8d1561688ecf5b7b5
> 3rdparty/libprocess/include/process/future.hpp 5e124760ca304082da506c91b6341e784f103d93
> 3rdparty/libprocess/include/process/socket.hpp dbcb4f4c2eb12663158057a844b4511d6dde0508
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/process.cpp 6c6acc03ad78779e8f25b1a4584069fd377f647b
> bin/mesos-slave-flags.sh.in 8c936aa06e994a87a8b09b31c907868bf9be38c7
> configure.ac 9a6de87fa6523bf8137f5d74ea0b7c6cbd947d3a
> docs/configuration.md 2dbe0f81aea0e0db41f3eb4b0554ae5e9f13da2e
> docs/getting-started.md d7ced76125c6aba0d7e1a0f4ca8165652b17af0d
> src/Makefile.am 0775a0df293e945d41c7ba90fd1bbb503ae22f9e
> src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 668647fdfc0e203fcde59263256659ba14e29960
> src/linux/cgroups.hpp 6056e3ae59ed813d75b1bb46b276872530155ec7
> src/linux/cgroups.cpp eff15af417f3827b4a1f73770b98dc9fccbc397e
> src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
> src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
> src/local/local.cpp 2cfdf49c9eb92302502eb50c623f9606977b88b6
> src/log/log.cpp d9b26871bfa1b3c36f5e7d879eb7b67132af1b33
> src/master/detector.cpp 8b10061de12a35ad6624db594dc4ec36502b2420
> src/master/flags.hpp 024f86d93824a20ce42c28b8264576f1cb715d0e
> src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
> src/master/main.cpp f12f20a1eabd163c4f35056bf01f28f3edd408a9
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/sched/sched.cpp 3684cfe9d153cab5f4ea86094fffd814ce74baa1
> src/slave/containerizer/containerizer.cpp faf3d0cf97aafd88eab82ae39e2255b1eb4ce294
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 11665dbe88e3920fd9d8a2259987611d49e85462
> src/slave/containerizer/mesos_containerizer.cpp c819c97d96d232a1de3c1ed2fc848bebf66981f7
> src/slave/slave.cpp d8d3e0fa54972201d72b2650ec0ba922a4912d54
> src/slave/status_update_manager.cpp 5d5cf234ef2dd47fa4b1f67be761dbca31659451
> src/tests/cluster.hpp 40d9f8c18307aead2374396710f9a82466e3a716
> src/tests/environment.cpp feeca042265eede40e87db3ee5ab89be04509a90
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
> src/tests/state_tests.cpp d0e084070c566ee7d751a8e1279772e05b966145
>
> Diff: https://reviews.apache.org/r/19208/diff/
>
>
> Testing
> -------
>
> make check
>
> ran locally and connected to :5050 endpoint and watched log
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19208: Move HTTP request logging to ProcessBase and add
peer IP address.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On March 14, 2014, 10:23 a.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 3207-3208
> > <https://reviews.apache.org/r/19208/diff/1/?file=519368#file519368line3207>
> >
> > We avoid doing LOG(INFO) in our 3rd party libraries. Convert this to VLOG instead.
> >
> > More importantly, at Twitter we have observed that these LOGs are actually useful during debugging and hence we wanted these to be always present (i.e., LOG(INFO) instead of VLOG). That's likely the reason these are in http.cpp files.
> >
>
> Dominic Hamon wrote:
> This doesn't add any more log-spam than we already had as we were logging this anyway, albeit without the IP address. We can't easily expose the IP address down to the http files (and probably shouldn't) whereas this is really useful debugging information.
>
> This should be a LOG(INFO), or at least a log to a separate HTTP access log file if the main log is too sensitive to it.
>
> Vinod Kone wrote:
> Rebase?
>
> More importantly, this is not an issue of log spam. We have deliberately avoided adding LOG(INFO) statements in stout and libprocess (the only violation i see is in the profiler but that should be fixed).
>
> ? ack "LOG\(INFO" ../3rdparty/libprocess/
> ../3rdparty/libprocess/include/process/profiler.hpp
> 58: LOG(INFO) << "Starting Profiler";
> 94: LOG(INFO) << "Stopping Profiler";
> ? ack "LOG\(INFO" ../3rdparty/libprocess/3rdparty/stout
> ? ?
>
>
> How about adding the sender ip/hostname as a member in "http::Request"? Not sure if thats the right place. BenM?
>
Adding sender IP/hostname to http::Request sounds reasonable, though it does mean that every endpoint is responsible for logging the info. For something so important to debugging, that seems fragile.
Is the concern about having LOG(INFO) in libprocess that it will get out of hand?
Every HTTP server I know of has some kind of default access logging, precisely because it is vital information. Moving it to a custom log is doable, but requires infrastructure around log rotation that may be a pain to deal with.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19208/#review37220
-----------------------------------------------------------
On March 19, 2014, 3:49 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19208/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 3:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-1096
> https://issues.apache.org/jira/browse/MESOS-1096
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am df149ddc8669b0c541361922631f4f3958bd096d
> 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 47fe92c380de3e2abc625dc936afbd034280b76a
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp ba8463cc152701d6eb1deba8d1561688ecf5b7b5
> 3rdparty/libprocess/include/process/future.hpp 5e124760ca304082da506c91b6341e784f103d93
> 3rdparty/libprocess/include/process/socket.hpp dbcb4f4c2eb12663158057a844b4511d6dde0508
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/process.cpp 6c6acc03ad78779e8f25b1a4584069fd377f647b
> bin/mesos-slave-flags.sh.in 8c936aa06e994a87a8b09b31c907868bf9be38c7
> configure.ac 9a6de87fa6523bf8137f5d74ea0b7c6cbd947d3a
> docs/configuration.md 2dbe0f81aea0e0db41f3eb4b0554ae5e9f13da2e
> docs/getting-started.md d7ced76125c6aba0d7e1a0f4ca8165652b17af0d
> src/Makefile.am 0775a0df293e945d41c7ba90fd1bbb503ae22f9e
> src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 668647fdfc0e203fcde59263256659ba14e29960
> src/linux/cgroups.hpp 6056e3ae59ed813d75b1bb46b276872530155ec7
> src/linux/cgroups.cpp eff15af417f3827b4a1f73770b98dc9fccbc397e
> src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
> src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
> src/local/local.cpp 2cfdf49c9eb92302502eb50c623f9606977b88b6
> src/log/log.cpp d9b26871bfa1b3c36f5e7d879eb7b67132af1b33
> src/master/detector.cpp 8b10061de12a35ad6624db594dc4ec36502b2420
> src/master/flags.hpp 024f86d93824a20ce42c28b8264576f1cb715d0e
> src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
> src/master/main.cpp f12f20a1eabd163c4f35056bf01f28f3edd408a9
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/sched/sched.cpp 3684cfe9d153cab5f4ea86094fffd814ce74baa1
> src/slave/containerizer/containerizer.cpp faf3d0cf97aafd88eab82ae39e2255b1eb4ce294
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 11665dbe88e3920fd9d8a2259987611d49e85462
> src/slave/containerizer/mesos_containerizer.cpp c819c97d96d232a1de3c1ed2fc848bebf66981f7
> src/slave/slave.cpp d8d3e0fa54972201d72b2650ec0ba922a4912d54
> src/slave/status_update_manager.cpp 5d5cf234ef2dd47fa4b1f67be761dbca31659451
> src/tests/cluster.hpp 40d9f8c18307aead2374396710f9a82466e3a716
> src/tests/environment.cpp feeca042265eede40e87db3ee5ab89be04509a90
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
> src/tests/state_tests.cpp d0e084070c566ee7d751a8e1279772e05b966145
>
> Diff: https://reviews.apache.org/r/19208/diff/
>
>
> Testing
> -------
>
> make check
>
> ran locally and connected to :5050 endpoint and watched log
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19208: Move HTTP request logging to ProcessBase and add
peer IP address.
Posted by Ben Mahler <be...@gmail.com>.
> On March 14, 2014, 5:23 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 3207-3208
> > <https://reviews.apache.org/r/19208/diff/1/?file=519368#file519368line3207>
> >
> > We avoid doing LOG(INFO) in our 3rd party libraries. Convert this to VLOG instead.
> >
> > More importantly, at Twitter we have observed that these LOGs are actually useful during debugging and hence we wanted these to be always present (i.e., LOG(INFO) instead of VLOG). That's likely the reason these are in http.cpp files.
> >
>
> Dominic Hamon wrote:
> This doesn't add any more log-spam than we already had as we were logging this anyway, albeit without the IP address. We can't easily expose the IP address down to the http files (and probably shouldn't) whereas this is really useful debugging information.
>
> This should be a LOG(INFO), or at least a log to a separate HTTP access log file if the main log is too sensitive to it.
>
> Vinod Kone wrote:
> Rebase?
>
> More importantly, this is not an issue of log spam. We have deliberately avoided adding LOG(INFO) statements in stout and libprocess (the only violation i see is in the profiler but that should be fixed).
>
> ? ack "LOG\(INFO" ../3rdparty/libprocess/
> ../3rdparty/libprocess/include/process/profiler.hpp
> 58: LOG(INFO) << "Starting Profiler";
> 94: LOG(INFO) << "Stopping Profiler";
> ? ack "LOG\(INFO" ../3rdparty/libprocess/3rdparty/stout
> ? ?
>
>
> How about adding the sender ip/hostname as a member in "http::Request"? Not sure if thats the right place. BenM?
>
>
> Dominic Hamon wrote:
> Adding sender IP/hostname to http::Request sounds reasonable, though it does mean that every endpoint is responsible for logging the info. For something so important to debugging, that seems fragile.
>
> Is the concern about having LOG(INFO) in libprocess that it will get out of hand?
>
> Every HTTP server I know of has some kind of default access logging, precisely because it is vital information. Moving it to a custom log is doable, but requires infrastructure around log rotation that may be a pain to deal with.
I like Vinod's suggestion of adding this information into the http::Request and logging that information in our existing Mesos endpoints, that way we can have an immediate improvement without getting overly ambitious here. :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19208/#review37220
-----------------------------------------------------------
On March 19, 2014, 10:49 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19208/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 10:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-1096
> https://issues.apache.org/jira/browse/MESOS-1096
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am df149ddc8669b0c541361922631f4f3958bd096d
> 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 47fe92c380de3e2abc625dc936afbd034280b76a
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp ba8463cc152701d6eb1deba8d1561688ecf5b7b5
> 3rdparty/libprocess/include/process/future.hpp 5e124760ca304082da506c91b6341e784f103d93
> 3rdparty/libprocess/include/process/socket.hpp dbcb4f4c2eb12663158057a844b4511d6dde0508
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/process.cpp 6c6acc03ad78779e8f25b1a4584069fd377f647b
> bin/mesos-slave-flags.sh.in 8c936aa06e994a87a8b09b31c907868bf9be38c7
> configure.ac 9a6de87fa6523bf8137f5d74ea0b7c6cbd947d3a
> docs/configuration.md 2dbe0f81aea0e0db41f3eb4b0554ae5e9f13da2e
> docs/getting-started.md d7ced76125c6aba0d7e1a0f4ca8165652b17af0d
> src/Makefile.am 0775a0df293e945d41c7ba90fd1bbb503ae22f9e
> src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 668647fdfc0e203fcde59263256659ba14e29960
> src/linux/cgroups.hpp 6056e3ae59ed813d75b1bb46b276872530155ec7
> src/linux/cgroups.cpp eff15af417f3827b4a1f73770b98dc9fccbc397e
> src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
> src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
> src/local/local.cpp 2cfdf49c9eb92302502eb50c623f9606977b88b6
> src/log/log.cpp d9b26871bfa1b3c36f5e7d879eb7b67132af1b33
> src/master/detector.cpp 8b10061de12a35ad6624db594dc4ec36502b2420
> src/master/flags.hpp 024f86d93824a20ce42c28b8264576f1cb715d0e
> src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
> src/master/main.cpp f12f20a1eabd163c4f35056bf01f28f3edd408a9
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/sched/sched.cpp 3684cfe9d153cab5f4ea86094fffd814ce74baa1
> src/slave/containerizer/containerizer.cpp faf3d0cf97aafd88eab82ae39e2255b1eb4ce294
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 11665dbe88e3920fd9d8a2259987611d49e85462
> src/slave/containerizer/mesos_containerizer.cpp c819c97d96d232a1de3c1ed2fc848bebf66981f7
> src/slave/slave.cpp d8d3e0fa54972201d72b2650ec0ba922a4912d54
> src/slave/status_update_manager.cpp 5d5cf234ef2dd47fa4b1f67be761dbca31659451
> src/tests/cluster.hpp 40d9f8c18307aead2374396710f9a82466e3a716
> src/tests/environment.cpp feeca042265eede40e87db3ee5ab89be04509a90
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
> src/tests/state_tests.cpp d0e084070c566ee7d751a8e1279772e05b966145
>
> Diff: https://reviews.apache.org/r/19208/diff/
>
>
> Testing
> -------
>
> make check
>
> ran locally and connected to :5050 endpoint and watched log
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19208: Move HTTP request logging to ProcessBase and add
peer IP address.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19208/#review37220
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/19208/#comment68642>
This should be abstracted away in stout.
Also, consider adding a method on Socket call that gives the IP of the remote end point.
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/19208/#comment68641>
We avoid doing LOG(INFO) in our 3rd party libraries. Convert this to VLOG instead.
More importantly, at Twitter we have observed that these LOGs are actually useful during debugging and hence we wanted these to be always present (i.e., LOG(INFO) instead of VLOG). That's likely the reason these are in http.cpp files.
- Vinod Kone
On March 14, 2014, 4:31 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19208/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 4:31 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-1096
> https://issues.apache.org/jira/browse/MESOS-1096
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 6c6acc03ad78779e8f25b1a4584069fd377f647b
> src/master/http.cpp a019f15615d028f3d3628b2611709fc8a3a81bf8
> src/slave/http.cpp 594032da1b2edd47961cd8201acd093b22fa30ae
>
> Diff: https://reviews.apache.org/r/19208/diff/
>
>
> Testing
> -------
>
> make check
>
> ran locally and connected to :5050 endpoint and watched log
>
>
> Thanks,
>
> Dominic Hamon
>
>