Alon Bar-Lev has uploaded a new change for review. Change subject: bll: use MDC to hold correlation id ......................................................................
bll: use MDC to hold correlation id apparently the slf4j within jboss and MDC support of the log handler are integrated, so we can levarage that in order to delegate correlation id to log without actually have our own logger. this finally move the correlation id into presentation only container, in future nothing should get correlation id out of it, as each command should have reference to its parent and can obtain the id from the chain. Bug-Url: https://bugzilla.redhat.com/1109871 Change-Id: Iff53d3bf420e6fe1b1a0a04a0a21a49a3c01a4d2 Signed-off-by: Alon Bar-Lev <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunner.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/interceptors/ThreadLocalSessionCleanerInterceptor.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/job/ExecutionHandler.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CommandBaseTest.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/QueriesCommandBaseTest.java A backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/CorrelationIdTracker.java D backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ThreadLocalParamsContainer.java M backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Log.java M backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/threadpool/ThreadPoolUtil.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/xmlrpc/XmlRpcUtils.java M packaging/services/ovirt-engine/ovirt-engine.xml.in 12 files changed, 71 insertions(+), 89 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/18/34218/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java index ca897ee..96f9f91 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java @@ -93,7 +93,7 @@ import org.ovirt.engine.core.utils.Deserializer; import org.ovirt.engine.core.utils.ReflectionUtils; import org.ovirt.engine.core.utils.SerializationFactory; -import org.ovirt.engine.core.utils.ThreadLocalParamsContainer; +import org.ovirt.engine.core.utils.CorrelationIdTracker; import org.ovirt.engine.core.utils.lock.EngineLock; import org.ovirt.engine.core.utils.lock.LockManager; import org.ovirt.engine.core.utils.lock.LockManagerFactory; @@ -2139,7 +2139,7 @@ public void setCorrelationId(String correlationId) { // correlation ID thread local variable is set for non multi-action if (!_parameters.getMultipleAction()) { - ThreadLocalParamsContainer.setCorrelationId(correlationId); + CorrelationIdTracker.setCorrelationId(correlationId); } super.setCorrelationId(correlationId); } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunner.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunner.java index ac516e2..4168282 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunner.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultipleActionsRunner.java @@ -9,7 +9,7 @@ import org.ovirt.engine.core.common.action.VdcActionParametersBase; import org.ovirt.engine.core.common.action.VdcActionType; import org.ovirt.engine.core.common.action.VdcReturnValueBase; -import org.ovirt.engine.core.utils.ThreadLocalParamsContainer; +import org.ovirt.engine.core.utils.CorrelationIdTracker; import org.ovirt.engine.core.utils.log.Log; import org.ovirt.engine.core.utils.log.LogFactory; import org.ovirt.engine.core.utils.threadpool.ThreadPoolUtil; @@ -74,7 +74,7 @@ } if (getCommands().size() == 1) { - ThreadLocalParamsContainer.setCorrelationId(getCommands().get(0).getCorrelationId()); + CorrelationIdTracker.setCorrelationId(getCommands().get(0).getCorrelationId()); returnValues.add(getCommands().get(0).canDoActionOnly()); } else { checkCanDoActionsAsynchronously(returnValues); @@ -137,7 +137,7 @@ protected VdcReturnValueBase runCanDoActionOnly(final int currentCanDoActionId, final int totalSize) { CommandBase<?> command = getCommands().get(currentCanDoActionId); String actionType = command.getActionType().toString(); - ThreadLocalParamsContainer.setCorrelationId(command.getCorrelationId()); + CorrelationIdTracker.setCorrelationId(command.getCorrelationId()); try { log.infoFormat("Start running CanDoAction for command number {0}/{1} (Command type: {2})", currentCanDoActionId + 1, @@ -186,7 +186,7 @@ command.getActionType(), command.isInternalExecution()); } - ThreadLocalParamsContainer.setCorrelationId(command.getCorrelationId()); + CorrelationIdTracker.setCorrelationId(command.getCorrelationId()); command.insertAsyncTaskPlaceHolders(); command.executeAction(); } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/interceptors/ThreadLocalSessionCleanerInterceptor.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/interceptors/ThreadLocalSessionCleanerInterceptor.java index 18f204b..1e8af69 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/interceptors/ThreadLocalSessionCleanerInterceptor.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/interceptors/ThreadLocalSessionCleanerInterceptor.java @@ -3,7 +3,7 @@ import javax.interceptor.AroundInvoke; import javax.interceptor.InvocationContext; -import org.ovirt.engine.core.utils.ThreadLocalParamsContainer; +import org.ovirt.engine.core.utils.CorrelationIdTracker; public class ThreadLocalSessionCleanerInterceptor { @@ -12,7 +12,7 @@ try { return ic.proceed(); } finally { - ThreadLocalParamsContainer.clean(); + CorrelationIdTracker.clean(); } } } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/job/ExecutionHandler.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/job/ExecutionHandler.java index f5e764a..41df5dd 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/job/ExecutionHandler.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/job/ExecutionHandler.java @@ -27,7 +27,7 @@ import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.dal.dbbroker.DbFacade; import org.ovirt.engine.core.dal.job.ExecutionMessageDirector; -import org.ovirt.engine.core.utils.ThreadLocalParamsContainer; +import org.ovirt.engine.core.utils.CorrelationIdTracker; import org.ovirt.engine.core.utils.lock.EngineLock; import org.ovirt.engine.core.utils.log.Log; import org.ovirt.engine.core.utils.log.LogFactory; @@ -729,7 +729,7 @@ VdcReturnValueBase returnValue = null; String correlationId = parameters.getCorrelationId(); if (StringUtils.isEmpty(correlationId)) { - correlationId = ThreadLocalParamsContainer.getCorrelationId(); + correlationId = CorrelationIdTracker.getCorrelationId(); if (StringUtils.isEmpty(correlationId)) { correlationId = LoggedUtils.getObjectId(parameters); } diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CommandBaseTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CommandBaseTest.java index 1a85958..d436c24 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CommandBaseTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CommandBaseTest.java @@ -34,7 +34,7 @@ import org.ovirt.engine.core.dao.BusinessEntitySnapshotDAO; import org.ovirt.engine.core.utils.MockConfigRule; import org.ovirt.engine.core.utils.MockEJBStrategyRule; -import org.ovirt.engine.core.utils.ThreadLocalParamsContainer; +import org.ovirt.engine.core.utils.CorrelationIdTracker; /** A test case for {@link CommandBase} */ public class CommandBaseTest { @@ -95,7 +95,7 @@ @Before @After public void clearEnvironment() { - ThreadLocalParamsContainer.clean(); + CorrelationIdTracker.clean(); SessionDataContainer.getInstance().removeSessionOnLogout(session); } diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/QueriesCommandBaseTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/QueriesCommandBaseTest.java index e1012bf..7b1ae98 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/QueriesCommandBaseTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/QueriesCommandBaseTest.java @@ -15,7 +15,7 @@ import org.ovirt.engine.core.common.queries.VdcQueryParametersBase; import org.ovirt.engine.core.common.queries.VdcQueryType; import org.ovirt.engine.core.compat.Guid; -import org.ovirt.engine.core.utils.ThreadLocalParamsContainer; +import org.ovirt.engine.core.utils.CorrelationIdTracker; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -136,7 +136,7 @@ @Before @After public void clearSession() { - ThreadLocalParamsContainer.clean(); + CorrelationIdTracker.clean(); } /** A stub class that will cause the {@link VdcQueryType#Unknown} to be used */ diff --git a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/CorrelationIdTracker.java b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/CorrelationIdTracker.java new file mode 100644 index 0000000..562949c --- /dev/null +++ b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/CorrelationIdTracker.java @@ -0,0 +1,32 @@ +package org.ovirt.engine.core.utils; + +import org.slf4j.MDC; + +public class CorrelationIdTracker { + + private static final String MDC_CORRELATION_ID = "ovirtCorrelationId"; + + /** + * Set the value of the correlation-ID of the current thread and the value to be printed in the logger and past to + * VDSM + * + * @param correlation + * The value of the correlation-ID to be logged + */ + public static void setCorrelationId(String correlation) { + if (correlation == null) { + MDC.remove(MDC_CORRELATION_ID); + } else { + MDC.put(MDC_CORRELATION_ID, correlation); + } + } + + public static String getCorrelationId() { + return MDC.get(MDC_CORRELATION_ID); + } + + public static void clean() { + MDC.remove(MDC_CORRELATION_ID); + } + +} diff --git a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ThreadLocalParamsContainer.java b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ThreadLocalParamsContainer.java deleted file mode 100644 index 5f0b555..0000000 --- a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ThreadLocalParamsContainer.java +++ /dev/null @@ -1,31 +0,0 @@ -package org.ovirt.engine.core.utils; - - - -public class ThreadLocalParamsContainer { - - /** - * Identifies the correlation-id associated with the current thread - */ - private static final ThreadLocal<String> correlationId = new ThreadLocal<String>(); - - /** - * Set the value of the correlation-ID of the current thread and the value to be printed in the logger and past to - * VDSM - * - * @param correlation - * The value of the correlation-ID to be logged - */ - public static void setCorrelationId(String correlation) { - correlationId.set(correlation); - } - - public static String getCorrelationId() { - return correlationId.get(); - } - - public static void clean() { - correlationId.remove(); - } - -} diff --git a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Log.java b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Log.java index 7604fd5..7e7f831 100644 --- a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Log.java +++ b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Log.java @@ -2,10 +2,7 @@ import java.text.MessageFormat; -import org.ovirt.engine.core.utils.ThreadLocalParamsContainer; - public class Log { - private static final String CORRELATION_ID_MESSAGE_FORMAT = "[%s] %s"; private final org.slf4j.Logger log; @@ -14,31 +11,27 @@ } public void debug(Object msg) { - if (isDebugEnabled()) { - log.debug(addPrefixToLogMessage(msg)); - } + log.debug(convertToString(msg)); } public void debug(Object msg, Throwable t) { - if (isDebugEnabled()) { - log.debug(addPrefixToLogMessage(msg), t); - } + log.debug(convertToString(msg), t); } public void error(Object msg) { - log.error(addPrefixToLogMessage(msg)); + log.error(convertToString(msg)); } public void error(Object msg, Throwable t) { - log.error(addPrefixToLogMessage(msg), t); + log.error(convertToString(msg), t); } public void info(Object msg) { - log.info(addPrefixToLogMessage(msg)); + log.info(convertToString(msg)); } public void info(Object msg, Throwable t) { - log.info(addPrefixToLogMessage(msg), t); + log.info(convertToString(msg), t); } public boolean isDebugEnabled() { @@ -66,23 +59,19 @@ } public void trace(Object msg) { - if (log.isTraceEnabled()) { - log.trace(addPrefixToLogMessage(msg)); - } + log.trace(convertToString(msg)); } public void trace(Object msg, Throwable t) { - if (log.isTraceEnabled()) { - log.trace(addPrefixToLogMessage(msg), t); - } + log.trace(convertToString(msg), t); } public void warn(Object msg) { - log.warn(addPrefixToLogMessage(msg)); + log.warn(convertToString(msg)); } public void warn(Object msg, Throwable t) { - log.warn(addPrefixToLogMessage(msg), t); + log.warn(convertToString(msg), t); } public void traceFormat(String formatString, Object... args) { @@ -154,16 +143,8 @@ return null; } - private String addPrefixToLogMessage(Object logMessage) { - String correlationId = ThreadLocalParamsContainer.getCorrelationId(); - if (correlationId == null) { - if (logMessage == null) { - return null; - } else { - return logMessage.toString(); - } - } - return String.format(CORRELATION_ID_MESSAGE_FORMAT, correlationId, logMessage); + private String convertToString(Object logMessage) { + return logMessage == null ? null : logMessage.toString(); } } diff --git a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/threadpool/ThreadPoolUtil.java b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/threadpool/ThreadPoolUtil.java index 4f7c1ed..0d251ab 100644 --- a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/threadpool/ThreadPoolUtil.java +++ b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/threadpool/ThreadPoolUtil.java @@ -16,7 +16,7 @@ import org.ovirt.engine.core.common.config.Config; import org.ovirt.engine.core.common.config.ConfigValues; -import org.ovirt.engine.core.utils.ThreadLocalParamsContainer; +import org.ovirt.engine.core.utils.CorrelationIdTracker; import org.ovirt.engine.core.utils.log.Log; import org.ovirt.engine.core.utils.log.LogFactory; @@ -61,7 +61,7 @@ @Override protected void afterExecute(Runnable r, Throwable t) { super.afterExecute(r, t); - ThreadLocalParamsContainer.clean(); + CorrelationIdTracker.clean(); } @Override @@ -86,7 +86,7 @@ @Override public void run() { - ThreadLocalParamsContainer.setCorrelationId(correlationId); + CorrelationIdTracker.setCorrelationId(correlationId); job.run(); } @@ -103,12 +103,12 @@ public InternalCallable(Callable<V> job) { this.job = job; - this.correlationId = ThreadLocalParamsContainer.getCorrelationId(); + this.correlationId = CorrelationIdTracker.getCorrelationId(); } @Override public V call() throws Exception { - ThreadLocalParamsContainer.setCorrelationId(correlationId); + CorrelationIdTracker.setCorrelationId(correlationId); return job.call(); } } @@ -152,7 +152,7 @@ public static void execute(Runnable command) { try { es.submit(new InternalWrapperRunnable(command, - ThreadLocalParamsContainer.getCorrelationId())); + CorrelationIdTracker.getCorrelationId())); } catch (RejectedExecutionException e) { log.warn("The thread pool is out of limit. A submitted task was rejected"); throw e; diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/xmlrpc/XmlRpcUtils.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/xmlrpc/XmlRpcUtils.java index efc6b90..0b5e819 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/xmlrpc/XmlRpcUtils.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/xmlrpc/XmlRpcUtils.java @@ -32,7 +32,7 @@ import org.ovirt.engine.core.common.config.Config; import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.common.utils.Pair; -import org.ovirt.engine.core.utils.ThreadLocalParamsContainer; +import org.ovirt.engine.core.utils.CorrelationIdTracker; import org.ovirt.engine.core.utils.crypt.EngineEncryptionUtils; import org.ovirt.engine.core.utils.log.Log; import org.ovirt.engine.core.utils.log.LogFactory; @@ -186,7 +186,7 @@ new FutureTask<Object>(createCallable(obj, getMethod(m, annotation, proxy), args, - ThreadLocalParamsContainer.getCorrelationId())); + CorrelationIdTracker.getCorrelationId())); ThreadPoolUtil.execute(future); return future; } else { @@ -194,7 +194,7 @@ new FutureTask<Object>(createCallable(obj, m, args, - ThreadLocalParamsContainer.getCorrelationId())); + CorrelationIdTracker.getCorrelationId())); ThreadPoolUtil.execute(future); try { result = future.get(timeoutInMilisec, TimeUnit.MILLISECONDS); @@ -238,7 +238,7 @@ @Override public Object call() throws Exception { try { - ThreadLocalParamsContainer.setCorrelationId(correlationId); + CorrelationIdTracker.setCorrelationId(correlationId); return m.invoke(obj, args); } catch (Exception e) { throw e; @@ -264,7 +264,7 @@ protected void initHttpHeaders(XmlRpcRequest pRequest) throws XmlRpcClientException { super.initHttpHeaders(pRequest); - String correlationId = ThreadLocalParamsContainer.getCorrelationId(); + String correlationId = CorrelationIdTracker.getCorrelationId(); if (StringUtils.isNotBlank(correlationId)) { method.setRequestHeader(FLOW_ID_HEADER_NAME, correlationId); } diff --git a/packaging/services/ovirt-engine/ovirt-engine.xml.in b/packaging/services/ovirt-engine/ovirt-engine.xml.in index 66220c4..fbba615 100644 --- a/packaging/services/ovirt-engine/ovirt-engine.xml.in +++ b/packaging/services/ovirt-engine/ovirt-engine.xml.in @@ -80,7 +80,7 @@ <file-handler name="ENGINE" autoflush="true"> <level name="INFO"/> <formatter> - <pattern-formatter pattern="%d %-5p [%c] (%t) %s%E%n"/> + <pattern-formatter pattern="%d %-5p [%c] (%t) [%X{ovirtCorrelationId}] %s%E%n"/> </formatter> <file path="$getstring('ENGINE_LOG')/engine.log"/> <append value="true"/> @@ -90,7 +90,7 @@ <console-handler name="CONSOLE" autoflush="true"> <level name="INFO"/> <formatter> - <pattern-formatter pattern="%d %-5p [%c] (%t) %s%E%n"/> + <pattern-formatter pattern="%d %-5p [%c] (%t) [%X{ovirtCorrelationId}] %s%E%n"/> </formatter> </console-handler> -- To view, visit http://gerrit.ovirt.org/34218 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Iff53d3bf420e6fe1b1a0a04a0a21a49a3c01a4d2 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
