opendev/gerrit
Code Issues Proposed changes

Refactor system log creation to remove code duplication

Browse Source 
Error, gc, http and ssh logs are created programmatically but the
code to create the appender was duplicated.

Move appender creation code into SystemLog to remove
duplicated code and allow to reuse it to create another log.

Also move LogUtil.shouldConfigureLogSystem into SystemLog class.

Change-Id: Ic691e573c3bcd74b4cb77905a176773540235f1a
This commit is contained in:
Hugo Arès
2014-06-26 15:58:53 -04:00
committed by David Pursehouse
parent bfdcec0071
commit c27f21d143
10 changed files with 165 additions and 399 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
gerrit-pgm/BUCK
View File

@@ -112,6 +112,7 @@ java_library(
'//lib:h2',
'//lib:servlet-api-3_1',
'//lib/guice:guice',
'//lib/guice:guice-assistedinject',
'//lib/guice:guice-servlet',
'//lib/jetty:server',
'//lib/jetty:servlet',

123
gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/HttpLog.java
View File

@@ -16,31 +16,28 @@ package com.google.gerrit.pgm.http.jetty;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.util.LogUtil;
import com.google.gerrit.server.util.SystemLog;
import com.google.gerrit.server.util.TimeUtil;
import com.google.inject.Inject;
import org.apache.log4j.Appender;
import org.apache.log4j.AsyncAppender;
import org.apache.log4j.DailyRollingFileAppender;
import org.apache.log4j.Level;
import org.apache.log4j.Logger;
import org.apache.log4j.spi.ErrorHandler;
import org.apache.log4j.spi.LoggingEvent;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.RequestLog;
import org.eclipse.jetty.server.Response;
import org.eclipse.jetty.util.component.AbstractLifeCycle;
import org.eclipse.jgit.lib.Config;
import java.io.File;
import java.io.IOException;
/** Writes the {@code httpd_log} file with per-request data. */
class HttpLog extends AbstractLifeCycle implements RequestLog {
private static final Logger log = Logger.getLogger(HttpLog.class);
private static final String LOG_NAME = "httpd_log";
interface HttpLogFactory {
HttpLog get();
}
protected static final String P_HOST = "Host";
protected static final String P_USER = "User";
protected static final String P_METHOD = "Method";
@@ -53,35 +50,9 @@ class HttpLog extends AbstractLifeCycle implements RequestLog {
private final AsyncAppender async;
HttpLog(final SitePaths site, final Config config) {
async = new AsyncAppender();
async.setBlocking(true);
async.setBufferSize(config.getInt("core", "asyncLoggingBufferSize", 64));
async.setLocationInfo(false);
if (LogUtil.shouldConfigureLogSystem()) {
final DailyRollingFileAppender dst = new DailyRollingFileAppender();
dst.setName(LOG_NAME);
dst.setLayout(new HttpLogLayout());
dst.setEncoding("UTF-8");
dst.setFile(new File(resolve(site.logs_dir), LOG_NAME).getPath());
dst.setImmediateFlush(true);
dst.setAppend(true);
dst.setThreshold(Level.INFO);
dst.setErrorHandler(new DieErrorHandler());
dst.activateOptions();
dst.setErrorHandler(new LogLogHandler());
async.addAppender(dst);
} else {
Appender appender = log.getAppender(LOG_NAME);
if (appender != null) {
async.addAppender(appender);
} else {
log.warn("No appender with the name: "
+ LOG_NAME
+ " was found. HTTPD logging is disabled");
}
}
async.activateOptions();
@Inject
HttpLog(final SystemLog systemLog) {
async = systemLog.createAsyncAppender(LOG_NAME, new HttpLogLayout());
}
@Override
@@ -151,80 +122,4 @@ class HttpLog extends AbstractLifeCycle implements RequestLog {
event.setProperty(key, String.valueOf(val));
}
}
private static File resolve(final File logs_dir) {
try {
return logs_dir.getCanonicalFile();
} catch (IOException e) {
return logs_dir.getAbsoluteFile();
}
}
private static final class DieErrorHandler implements ErrorHandler {
@Override
public void error(String message, Exception e, int errorCode,
LoggingEvent event) {
error(e != null ? e.getMessage() : message);
}
@Override
public void error(String message, Exception e, int errorCode) {
error(e != null ? e.getMessage() : message);
}
@Override
public void error(String message) {
throw new RuntimeException("Cannot open log file: " + message);
}
@Override
public void activateOptions() {
}
@Override
public void setAppender(Appender appender) {
}
@Override
public void setBackupAppender(Appender appender) {
}
@Override
public void setLogger(Logger logger) {
}
}
private static final class LogLogHandler implements ErrorHandler {
@Override
public void error(String message, Exception e, int errorCode,
LoggingEvent event) {
log.error(message, e);
}
@Override
public void error(String message, Exception e, int errorCode) {
log.error(message, e);
}
@Override
public void error(String message) {
log.error(message);
}
@Override
public void activateOptions() {
}
@Override
public void setAppender(Appender appender) {
}
@Override
public void setBackupAppender(Appender appender) {
}
@Override
public void setLogger(Logger logger) {
}
}
}

3
gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyModule.java
View File

@@ -15,6 +15,8 @@
package com.google.gerrit.pgm.http.jetty;
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.pgm.http.jetty.HttpLog.HttpLogFactory;
import com.google.inject.assistedinject.FactoryModuleBuilder;
public class JettyModule extends LifecycleModule {
private final JettyEnv env;
@@ -28,5 +30,6 @@ public class JettyModule extends LifecycleModule {
bind(JettyEnv.class).toInstance(env);
bind(JettyServer.class);
listener().to(JettyServer.Lifecycle.class);
install(new FactoryModuleBuilder().build(HttpLogFactory.class));
}
}

5
gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java
View File

@@ -25,6 +25,7 @@ import com.google.common.html.HtmlEscapers;
import com.google.common.io.ByteStreams;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.launcher.GerritLauncher;
import com.google.gerrit.pgm.http.jetty.HttpLog.HttpLogFactory;
import com.google.gerrit.reviewdb.client.AuthType;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -160,7 +161,7 @@ public class JettyServer {
@Inject
JettyServer(@GerritServerConfig final Config cfg, final SitePaths site,
final JettyEnv env)
final JettyEnv env, final HttpLogFactory httpLogFactory)
throws MalformedURLException, IOException {
this.site = site;
@@ -170,7 +171,7 @@ public class JettyServer {
Handler app = makeContext(env, cfg);
if (cfg.getBoolean("httpd", "requestLog", !reverseProxy)) {
RequestLogHandler handler = new RequestLogHandler();
handler.setRequestLog(new HttpLog(site, cfg));
handler.setRequestLog(httpLogFactory.get());
handler.setHandler(app);
app = handler;
}

73
gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/ErrorLogFile.java
View File

@@ -17,22 +17,16 @@ package com.google.gerrit.pgm.util;
import com.google.gerrit.common.Die;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.util.LogUtil;
import com.google.gerrit.server.util.SystemLog;
import org.apache.log4j.Appender;
import org.apache.log4j.ConsoleAppender;
import org.apache.log4j.DailyRollingFileAppender;
import org.apache.log4j.Level;
import org.apache.log4j.LogManager;
import org.apache.log4j.Logger;
import org.apache.log4j.PatternLayout;
import org.apache.log4j.helpers.OnlyOnceErrorHandler;
import org.apache.log4j.spi.ErrorHandler;
import org.apache.log4j.spi.LoggingEvent;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
public class ErrorLogFile {
static final String LOG_NAME = "error_log";
@@ -59,7 +53,7 @@ public class ErrorLogFile {
if (!logdir.exists() && !logdir.mkdirs()) {
throw new Die("Cannot create log directory: " + logdir);
}
if (LogUtil.shouldConfigureLogSystem()) {
if (SystemLog.shouldConfigure()) {
initLogSystem(logdir);
}
@@ -76,68 +70,9 @@ public class ErrorLogFile {
}
private static void initLogSystem(final File logdir) {
final PatternLayout layout = new PatternLayout();
layout.setConversionPattern("[%d] %-5p %c %x: %m%n");
final DailyRollingFileAppender dst = new DailyRollingFileAppender();
dst.setName(LOG_NAME);
dst.setLayout(layout);
dst.setEncoding("UTF-8");
dst.setFile(new File(resolve(logdir), LOG_NAME).getPath());
dst.setImmediateFlush(true);
dst.setAppend(true);
dst.setThreshold(Level.INFO);
dst.setErrorHandler(new DieErrorHandler());
dst.activateOptions();
dst.setErrorHandler(new OnlyOnceErrorHandler());
final Logger root = LogManager.getRootLogger();
root.removeAllAppenders();
root.addAppender(dst);
}
private static File resolve(final File logs_dir) {
try {
return logs_dir.getCanonicalFile();
} catch (IOException e) {
return logs_dir.getAbsoluteFile();
}
}
private ErrorLogFile() {
}
private static final class DieErrorHandler implements ErrorHandler {
@Override
public void error(String message, Exception e, int errorCode,
LoggingEvent event) {
error(e != null ? e.getMessage() : message);
}
@Override
public void error(String message, Exception e, int errorCode) {
error(e != null ? e.getMessage() : message);
}
@Override
public void error(String message) {
throw new Die("Cannot open log file: " + message);
}
@Override
public void activateOptions() {
}
@Override
public void setAppender(Appender appender) {
}
@Override
public void setBackupAppender(Appender appender) {
}
@Override
public void setLogger(Logger logger) {
}
root.addAppender(SystemLog.createAppender(logdir, LOG_NAME,
new PatternLayout("[%d] %-5p %c %x: %m%n")));
}
}

77
gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/GarbageCollectionLogFile.java
View File

@@ -18,25 +18,16 @@ import com.google.gerrit.common.Die;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.git.GarbageCollection;
import com.google.gerrit.server.util.LogUtil;
import com.google.gerrit.server.util.SystemLog;
import org.apache.log4j.Appender;
import org.apache.log4j.DailyRollingFileAppender;
import org.apache.log4j.Level;
import org.apache.log4j.LogManager;
import org.apache.log4j.Logger;
import org.apache.log4j.PatternLayout;
import org.apache.log4j.helpers.OnlyOnceErrorHandler;
import org.apache.log4j.spi.ErrorHandler;
import org.apache.log4j.spi.LoggingEvent;
import org.slf4j.LoggerFactory;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
public class GarbageCollectionLogFile {
private static final org.slf4j.Logger log = LoggerFactory.getLogger(GarbageCollectionLogFile.class);
public static LifecycleListener start(File sitePath)
throws FileNotFoundException {
@@ -44,7 +35,7 @@ public class GarbageCollectionLogFile {
if (!logdir.exists() && !logdir.mkdirs()) {
throw new Die("Cannot create log directory: " + logdir);
}
if (LogUtil.shouldConfigureLogSystem()) {
if (SystemLog.shouldConfigure()) {
initLogSystem(logdir);
}
@@ -61,70 +52,10 @@ public class GarbageCollectionLogFile {
}
private static void initLogSystem(File logdir) {
PatternLayout layout = new PatternLayout();
layout.setConversionPattern("[%d] %-5p %x: %m%n");
DailyRollingFileAppender dst = new DailyRollingFileAppender();
dst.setName(GarbageCollection.LOG_NAME);
dst.setLayout(layout);
dst.setEncoding("UTF-8");
dst.setFile(new File(resolve(logdir), GarbageCollection.LOG_NAME).getPath());
dst.setImmediateFlush(true);
dst.setAppend(true);
dst.setThreshold(Level.INFO);
dst.setErrorHandler(new LogErrorHandler());
dst.activateOptions();
dst.setErrorHandler(new OnlyOnceErrorHandler());
Logger gcLogger = LogManager.getLogger(GarbageCollection.LOG_NAME);
gcLogger.removeAllAppenders();
gcLogger.addAppender(dst);
gcLogger.addAppender(SystemLog.createAppender(logdir,
GarbageCollection.LOG_NAME, new PatternLayout("[%d] %-5p %x: %m%n")));
gcLogger.setAdditivity(false);
}
private static File resolve(File logs_dir) {
try {
return logs_dir.getCanonicalFile();
} catch (IOException e) {
return logs_dir.getAbsoluteFile();
}
}
private GarbageCollectionLogFile() {
}
private static final class LogErrorHandler implements ErrorHandler {
@Override
public void error(String message, Exception e, int errorCode,
LoggingEvent event) {
error(e != null ? e.getMessage() : message);
}
@Override
public void error(String message, Exception e, int errorCode) {
error(e != null ? e.getMessage() : message);
}
@Override
public void error(String message) {
log.error("Cannot open '" + GarbageCollection.LOG_NAME + "' log file: "
+ message);
}
@Override
public void activateOptions() {
}
@Override
public void setAppender(Appender appender) {
}
@Override
public void setBackupAppender(Appender appender) {
}
@Override
public void setLogger(Logger logger) {
}
}
}

1
gerrit-server/BUCK
View File

@@ -62,6 +62,7 @@ java_library(
'//lib/jgit:jgit-archive',
'//lib/joda:joda-time',
'//lib/log:api',
'//lib/log:log4j',
'//lib/prolog:prolog-cafe',
'//lib/lucene:analyzers-common',
'//lib/lucene:core',

26
gerrit-server/src/main/java/com/google/gerrit/server/util/LogUtil.java
View File

@@ -1,26 +0,0 @@
// Copyright (C) 2013 The Android Open Source Project
//
// 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.
package com.google.gerrit.server.util;
import com.google.common.base.Strings;
public class LogUtil {
private static final String LOG4J_CONFIGURATION = "log4j.configuration";
public static boolean shouldConfigureLogSystem() {
return Strings.isNullOrEmpty(System.getProperty(LOG4J_CONFIGURATION));
}
}

136
gerrit-server/src/main/java/com/google/gerrit/server/util/SystemLog.java Normal file
View File

@@ -0,0 +1,136 @@
// Copyright (C) 2014 The Android Open Source Project
//
// 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.
package com.google.gerrit.server.util;
import com.google.common.base.Strings;
import com.google.gerrit.common.Die;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import org.apache.log4j.Appender;
import org.apache.log4j.AsyncAppender;
import org.apache.log4j.DailyRollingFileAppender;
import org.apache.log4j.Layout;
import org.apache.log4j.Level;
import org.apache.log4j.LogManager;
import org.apache.log4j.Logger;
import org.apache.log4j.helpers.OnlyOnceErrorHandler;
import org.apache.log4j.spi.ErrorHandler;
import org.apache.log4j.spi.LoggingEvent;
import org.eclipse.jgit.lib.Config;
import org.slf4j.LoggerFactory;
import java.io.File;
import java.io.IOException;
@Singleton
public class SystemLog {
private static final org.slf4j.Logger log = LoggerFactory
.getLogger(SystemLog.class);
private static final String LOG4J_CONFIGURATION = "log4j.configuration";
private final SitePaths site;
private final Config config;
@Inject
public SystemLog(final SitePaths site, @GerritServerConfig Config config) {
this.site = site;
this.config = config;
}
public static boolean shouldConfigure() {
return Strings.isNullOrEmpty(System.getProperty(LOG4J_CONFIGURATION));
}
public static Appender createAppender(File logdir, String name, Layout layout) {
final DailyRollingFileAppender dst = new DailyRollingFileAppender();
dst.setName(name);
dst.setLayout(layout);
dst.setEncoding("UTF-8");
dst.setFile(new File(resolve(logdir), name).getPath());
dst.setImmediateFlush(true);
dst.setAppend(true);
dst.setThreshold(Level.INFO);
dst.setErrorHandler(new DieErrorHandler());
dst.activateOptions();
dst.setErrorHandler(new OnlyOnceErrorHandler());
return dst;
}
public AsyncAppender createAsyncAppender(String name, Layout layout) {
AsyncAppender async = new AsyncAppender();
async.setBlocking(true);
async.setBufferSize(config.getInt("core", "asyncLoggingBufferSize", 64));
async.setLocationInfo(false);
if (shouldConfigure()) {
async.addAppender(createAppender(site.logs_dir, name, layout));
} else {
Appender appender = LogManager.getLogger(name).getAppender(name);
if (appender != null) {
async.addAppender(appender);
} else {
log.warn("No appender with the name: " + name + " was found. " + name
+ " logging is disabled");
}
}
async.activateOptions();
return async;
}
private static File resolve(final File logs_dir) {
try {
return logs_dir.getCanonicalFile();
} catch (IOException e) {
return logs_dir.getAbsoluteFile();
}
}
private static final class DieErrorHandler implements ErrorHandler {
@Override
public void error(String message, Exception e, int errorCode,
LoggingEvent event) {
error(e != null ? e.getMessage() : message);
}
@Override
public void error(String message, Exception e, int errorCode) {
error(e != null ? e.getMessage() : message);
}
@Override
public void error(String message) {
throw new Die("Cannot open log file: " + message);
}
@Override
public void activateOptions() {
}
@Override
public void setAppender(Appender appender) {
}
@Override
public void setBackupAppender(Appender appender) {
}
@Override
public void setLogger(Logger logger) {
}
}
}

119
gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java
View File

@@ -23,27 +23,20 @@ import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PeerDaemonUser;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.util.IdGenerator;
import com.google.gerrit.server.util.LogUtil;
import com.google.gerrit.server.util.SystemLog;
import com.google.gerrit.server.util.TimeUtil;
import com.google.gerrit.sshd.SshScope.Context;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import org.apache.log4j.Appender;
import org.apache.log4j.AsyncAppender;
import org.apache.log4j.DailyRollingFileAppender;
import org.apache.log4j.Level;
import org.apache.log4j.Logger;
import org.apache.log4j.spi.ErrorHandler;
import org.apache.log4j.spi.LoggingEvent;
import org.eclipse.jgit.lib.Config;
import java.io.File;
import java.io.IOException;
@Singleton
class SshLog implements LifecycleListener {
private static final Logger log = Logger.getLogger(SshLog.class);
@@ -62,7 +55,8 @@ class SshLog implements LifecycleListener {
@Inject
SshLog(final Provider<SshSession> session, final Provider<Context> context,
final SitePaths site, @GerritServerConfig Config config, AuditService auditService) {
SystemLog systemLog, @GerritServerConfig Config config,
AuditService auditService) {
this.session = session;
this.context = context;
this.auditService = auditService;
@@ -71,36 +65,7 @@ class SshLog implements LifecycleListener {
async = null;
return;
}
async = new AsyncAppender();
async.setBlocking(true);
async.setBufferSize(config.getInt("core", "asyncLoggingBufferSize", 64));
async.setLocationInfo(false);
if (LogUtil.shouldConfigureLogSystem()) {
final DailyRollingFileAppender dst = new DailyRollingFileAppender();
dst.setName(LOG_NAME);
dst.setLayout(new SshLogLayout());
dst.setEncoding("UTF-8");
dst.setFile(new File(resolve(site.logs_dir), LOG_NAME).getPath());
dst.setImmediateFlush(true);
dst.setAppend(true);
dst.setThreshold(Level.INFO);
dst.setErrorHandler(new DieErrorHandler());
dst.activateOptions();
dst.setErrorHandler(new LogLogHandler());
async.addAppender(dst);
} else {
Appender appender = log.getAppender(LOG_NAME);
if (appender != null) {
async.addAppender(appender);
} else {
log.warn("No appender with the name: "
+ LOG_NAME
+ " was found. SSHD logging is disabled");
}
}
async.activateOptions();
async = systemLog.createAsyncAppender(LOG_NAME, new SshLogLayout());
}
@Override
@@ -276,82 +241,6 @@ class SshLog implements LifecycleListener {
return IdGenerator.format(id);
}
private static File resolve(final File logs_dir) {
try {
return logs_dir.getCanonicalFile();
} catch (IOException e) {
return logs_dir.getAbsoluteFile();
}
}
private static final class DieErrorHandler implements ErrorHandler {
@Override
public void error(String message, Exception e, int errorCode,
LoggingEvent event) {
error(e != null ? e.getMessage() : message);
}
@Override
public void error(String message, Exception e, int errorCode) {
error(e != null ? e.getMessage() : message);
}
@Override
public void error(String message) {
throw new RuntimeException("Cannot open log file: " + message);
}
@Override
public void activateOptions() {
}
@Override
public void setAppender(Appender appender) {
}
@Override
public void setBackupAppender(Appender appender) {
}
@Override
public void setLogger(Logger logger) {
}
}
private static final class LogLogHandler implements ErrorHandler {
@Override
public void error(String message, Exception e, int errorCode,
LoggingEvent event) {
log.error(message, e);
}
@Override
public void error(String message, Exception e, int errorCode) {
log.error(message, e);
}
@Override
public void error(String message) {
log.error(message);
}
@Override
public void activateOptions() {
}
@Override
public void setAppender(Appender appender) {
}
@Override
public void setBackupAppender(Appender appender) {
}
@Override
public void setLogger(Logger logger) {
}
}
void audit(Context ctx, Object result, String cmd) {
final String sid = extractSessionId(ctx);
final long created = extractCreated(ctx);