> 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 > >
