From 66c177c3b200670a43cfd970a138c84a037b4b7b Mon Sep 17 00:00:00 2001 From: Jeff Kaufman Date: Wed, 10 Apr 2013 14:29:08 -0400 Subject: [PATCH] logging: properly interpret google-style LOG() levels --- config | 7 +- src/log_message_handler.cc | 116 ++++++++++++++++++++++++++++++ src/log_message_handler.h | 44 ++++++++++++ src/ngx_message_handler.cc | 6 +- src/ngx_pagespeed.cc | 1 + src/ngx_rewrite_driver_factory.cc | 3 + test/nginx_system_test.sh | 9 +++ 7 files changed, 182 insertions(+), 4 deletions(-) create mode 100644 src/log_message_handler.cc create mode 100644 src/log_message_handler.h diff --git a/config b/config index cb24fff27..d4cb3f6e2 100644 --- a/config +++ b/config @@ -129,7 +129,11 @@ if [ $ngx_found = yes ]; then $ps_src/ngx_server_context.h \ $ps_src/ngx_rewrite_options.h \ $ps_src/ngx_rewrite_driver_factory.h \ - $ps_src/ngx_thread_system.h" + $ps_src/ngx_thread_system.h \ + $ps_src/ngx_message_handler.h \ + $ps_src/pthread_shared_mem.h \ + $ps_src/ngx_request_context.h \ + $ps_src/log_message_handler.h" NGX_ADDON_SRCS="$NGX_ADDON_SRCS \ $ps_src/ngx_pagespeed.cc \ $ps_src/ngx_rewrite_driver_factory.cc \ @@ -142,6 +146,7 @@ if [ $ngx_found = yes ]; then $ps_src/ngx_message_handler.cc \ $ps_src/pthread_shared_mem.cc \ $ps_src/ngx_request_context.cc \ + $ps_src/log_message_handler.cc \ $mod_pagespeed_dir/net/instaweb/apache/add_headers_fetcher.cc \ $mod_pagespeed_dir/net/instaweb/apache/loopback_route_fetcher.cc \ $mod_pagespeed_dir/net/instaweb/apache/serf_url_async_fetcher.cc" diff --git a/src/log_message_handler.cc b/src/log_message_handler.cc new file mode 100644 index 000000000..4b82d8f49 --- /dev/null +++ b/src/log_message_handler.cc @@ -0,0 +1,116 @@ +// Copyright 2010 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// Author: jmarantz@google.com (Joshua Marantz) +// Author: sligocki@google.com (Shawn Ligocki) +// Author: jefftk@google.com (Jeff Kaufman) + +// TODO(jefftk): share more of this code with apache's log_message_handler + +#include "log_message_handler.h" + +#include + +#include +#include + +#include "base/debug/debugger.h" +#include "base/debug/stack_trace.h" +#include "base/logging.h" +#include "net/instaweb/public/version.h" +#include "net/instaweb/util/public/string_util.h" + +// Make sure we don't attempt to use LOG macros here, since doing so +// would cause us to go into an infinite log loop. +#undef LOG +#define LOG USING_LOG_HERE_WOULD_CAUSE_INFINITE_RECURSION + +namespace { + +ngx_log_t* log = NULL; + +ngx_uint_t GetNgxLogLevel(int severity) { + switch (severity) { + case logging::LOG_INFO: + return NGX_LOG_INFO; + case logging::LOG_WARNING: + return NGX_LOG_WARN; + case logging::LOG_ERROR: + return NGX_LOG_ERR; + case logging::LOG_ERROR_REPORT: + case logging::LOG_FATAL: + return NGX_LOG_ALERT; + default: // For VLOG(s) + return NGX_LOG_DEBUG; + } +} + +bool LogMessageHandler(int severity, const char* file, int line, + size_t message_start, const GoogleString& str) { + ngx_uint_t this_log_level = GetNgxLogLevel(severity); + + GoogleString message = str; + if (severity == logging::LOG_FATAL) { + if (base::debug::BeingDebugged()) { + base::debug::BreakDebugger(); + } else { + base::debug::StackTrace trace; + std::ostringstream stream; + trace.OutputToStream(&stream); + message.append(stream.str()); + } + } + + // Trim the newline off the end of the message string. + size_t last_msg_character_index = message.length() - 1; + if (message[last_msg_character_index] == '\n') { + message.resize(last_msg_character_index); + } + + ngx_log_error(this_log_level, log, 0, "[ngx_pagespeed %s] %s", + net_instaweb::kModPagespeedVersion, + message.c_str()); + + if (severity == logging::LOG_FATAL) { + // Crash the process to generate a dump. + base::debug::BreakDebugger(); + } + + return true; +} + +} // namespace + + +namespace net_instaweb { + +namespace log_message_handler { + + +const int kDebugLogLevel = -2; + +void Install(ngx_log_t* log_in) { + log = log_in; + logging::SetLogMessageHandler(&LogMessageHandler); + + // All VLOG(2) and higher will be displayed as DEBUG logs if the nginx log + // level is DEBUG. + if (log->log_level >= NGX_LOG_DEBUG) { + logging::SetMinLogLevel(-2); + } +} + +} // namespace log_message_handler + +} // namespace net_instaweb diff --git a/src/log_message_handler.h b/src/log_message_handler.h new file mode 100644 index 000000000..05f7277ad --- /dev/null +++ b/src/log_message_handler.h @@ -0,0 +1,44 @@ +// Copyright 2010 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// Author: jmarantz@google.com (Joshua Marantz) +// Author: sligocki@google.com (Shawn Ligocki) +// Author: jefftk@google.com (Jeff Kaufman) + +#ifndef LOG_MESSAGE_HANDLER_H_ +#define LOG_MESSAGE_HANDLER_H_ + +extern "C" { + #include + #include + #include + #include +} + +namespace net_instaweb { + +namespace log_message_handler { + +// Install a log message handler that routes LOG() messages to the +// server error log. Should be called once at startup. If server blocks define +// their own logging files you would expect that LOG() messages would be routed +// appropriately, but because logging is all done with global variables this +// isn't possible. +void Install(ngx_log_t* log_in); + +} // namespace log_message_handler + +} // namespace net_instaweb + +#endif // LOG_MESSAGE_HANDLER_H_ diff --git a/src/ngx_message_handler.cc b/src/ngx_message_handler.cc index 8a3fa8c1b..48b57eb8e 100644 --- a/src/ngx_message_handler.cc +++ b/src/ngx_message_handler.cc @@ -104,7 +104,7 @@ void NgxMessageHandler::set_buffer(SharedCircularBuffer* buff) { } void NgxMessageHandler::MessageVImpl(MessageType type, const char* msg, - va_list args) { + va_list args) { ngx_uint_t log_level = GetNgxLogLevel(type); GoogleString formatted_message = Format(msg, args); if (log_ != NULL) { @@ -136,8 +136,8 @@ void NgxMessageHandler::MessageVImpl(MessageType type, const char* msg, } void NgxMessageHandler::FileMessageVImpl(MessageType type, const char* file, - int line, const char* msg, - va_list args) { + int line, const char* msg, + va_list args) { ngx_uint_t log_level = GetNgxLogLevel(type); GoogleString formatted_message = Format(msg, args); if (log_ != NULL) { diff --git a/src/ngx_pagespeed.cc b/src/ngx_pagespeed.cc index 6d6fd4965..eef031973 100644 --- a/src/ngx_pagespeed.cc +++ b/src/ngx_pagespeed.cc @@ -29,6 +29,7 @@ extern "C" { #include #include #include + #include } #include diff --git a/src/ngx_rewrite_driver_factory.cc b/src/ngx_rewrite_driver_factory.cc index 63166c6fb..aca848c1d 100644 --- a/src/ngx_rewrite_driver_factory.cc +++ b/src/ngx_rewrite_driver_factory.cc @@ -20,6 +20,7 @@ #include +#include "log_message_handler.h" #include "ngx_message_handler.h" #include "ngx_rewrite_options.h" #include "ngx_thread_system.h" @@ -302,6 +303,8 @@ void NgxRewriteDriverFactory::SharedCircularBufferInit(bool is_root) { } void NgxRewriteDriverFactory::RootInit(ngx_log_t* log) { + net_instaweb::log_message_handler::Install(log); + ParentOrChildInit(log); // Let SystemCaches know about the various paths we have in configuration diff --git a/test/nginx_system_test.sh b/test/nginx_system_test.sh index 793c03eb7..de0fa0627 100755 --- a/test/nginx_system_test.sh +++ b/test/nginx_system_test.sh @@ -478,6 +478,15 @@ fetch_until $URL 'fgrep -c web.example.ssp.css' 1 # directly from the filesystem. fetch_until $URL 'fgrep -c file.exception.ssp.css' 1 +# Test that ngx_pagespeed keeps working after nginx gets a signal to reload the +# configuration. This is in the middle of tests so that significant work +# happens both before and after. +start_test "Reload config" + +check wget $EXAMPLE_ROOT/styles/W.rewrite_css_images.css.pagespeed.cf.Hash.css +check_simple "$NGINX_EXECUTABLE" -s reload -c "$PAGESPEED_CONF" +check wget $EXAMPLE_ROOT/styles/W.rewrite_css_images.css.pagespeed.cf.Hash.css + start_test ModPagespeedLoadFromFileMatch URL=$TEST_ROOT/load_from_file_match/index.html?ModPagespeedFilters=inline_css fetch_until $URL 'grep -c blue' 1