From 40d3f1243bd394ec023dfb8645c72e97cd7ae730 Mon Sep 17 00:00:00 2001 From: Carpe-Wang <78642589+Carpe-Wang@users.noreply.github.com> Date: Sun, 6 Jul 2025 08:57:39 -0400 Subject: [PATCH] feat: Add LogUtil wrapper and optimize logging comments (#3489) Co-authored-by: aias00 Co-authored-by: Calvin Co-authored-by: shown Co-authored-by: kangli Co-authored-by: tomsun28 --- ...lertDefineAbstractImExportServiceImpl.java | 6 + .../service/impl/AlibabaSmsClientImpl.java | 7 +- .../collector/collect/jmx/JmxCollectImpl.java | 17 +- .../apache/hertzbeat/common/util/LogUtil.java | 167 ++++++++++++++++++ .../hertzbeat/common/util/LogUtilTest.java | 117 ++++++++++++ 5 files changed, 306 insertions(+), 8 deletions(-) create mode 100644 hertzbeat-common/src/main/java/org/apache/hertzbeat/common/util/LogUtil.java create mode 100644 hertzbeat-common/src/test/java/org/apache/hertzbeat/common/util/LogUtilTest.java diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/AlertDefineAbstractImExportServiceImpl.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/AlertDefineAbstractImExportServiceImpl.java index 121e17a9a5..de7c85eb16 100644 --- a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/AlertDefineAbstractImExportServiceImpl.java +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/AlertDefineAbstractImExportServiceImpl.java @@ -28,6 +28,9 @@ import org.apache.hertzbeat.alert.dto.ExportAlertDefineDTO; import org.apache.hertzbeat.alert.service.AlertDefineImExportService; import org.apache.hertzbeat.alert.service.AlertDefineService; import org.apache.hertzbeat.common.entity.alerter.AlertDefine; +import org.apache.hertzbeat.common.util.LogUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.beans.BeanUtils; import org.springframework.context.annotation.Lazy; import org.springframework.util.CollectionUtils; @@ -41,12 +44,15 @@ public abstract class AlertDefineAbstractImExportServiceImpl implements AlertDef @Lazy private AlertDefineService alertDefineService; + private static final Logger logger = LoggerFactory.getLogger(AlertDefineAbstractImExportServiceImpl.class); + @Override public void importConfig(InputStream is) { var formList = parseImport(is) .stream() .map(this::convert) .toList(); + LogUtil.info(logger, "Importing alert defines from {0}", formList); if (!CollectionUtils.isEmpty(formList)) { formList.forEach(alertDefine -> { alertDefineService.validate(alertDefine, false); diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/AlibabaSmsClientImpl.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/AlibabaSmsClientImpl.java index 45024c1a74..93d0f290c2 100644 --- a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/AlibabaSmsClientImpl.java +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/AlibabaSmsClientImpl.java @@ -27,11 +27,14 @@ import org.apache.hertzbeat.common.entity.alerter.NoticeReceiver; import org.apache.hertzbeat.common.entity.alerter.NoticeTemplate; import org.apache.hertzbeat.common.support.exception.SendMessageException; import org.apache.hertzbeat.common.util.JsonUtil; +import org.apache.hertzbeat.common.util.LogUtil; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpPost; import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClients; import org.apache.http.util.EntityUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.nio.charset.StandardCharsets; import java.text.SimpleDateFormat; @@ -62,6 +65,7 @@ public class AlibabaSmsClientImpl implements SmsClient { private final String accessKeySecret; private final String signName; private final String templateCode; + private static final Logger logger = LoggerFactory.getLogger(AlibabaSmsClientImpl.class); public AlibabaSmsClientImpl(AlibabaSmsProperties config) { if (config != null) { @@ -173,7 +177,7 @@ public class AlibabaSmsClientImpl implements SmsClient { log.info("Successfully sent SMS to phone: {}", phoneNumber); } } catch (Exception e) { - log.warn("Failed to send SMS: {}", e.getMessage()); + LogUtil.warn(logger, "Failed to send SMS: {0}", e.getMessage()); throw new SendMessageException(e.getMessage()); } } @@ -192,6 +196,7 @@ public class AlibabaSmsClientImpl implements SmsClient { // Step 4: Build authorization header return ALGORITHM + " Credential=" + accessKeyId + ",SignedHeaders=host;x-acs-action;x-acs-content-sha256;x-acs-date;" + "x-acs-signature-nonce;x-acs-version,Signature=" + signature; } catch (Exception e) { + LogUtil.warn(logger, "Failed to calculate authorization {0}", e.getMessage()); throw new RuntimeException("Failed to calculate authorization", e); } } diff --git a/hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/jmx/JmxCollectImpl.java b/hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/jmx/JmxCollectImpl.java index afd97a1801..2995469877 100644 --- a/hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/jmx/JmxCollectImpl.java +++ b/hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/jmx/JmxCollectImpl.java @@ -42,7 +42,6 @@ import javax.management.remote.JMXServiceURL; import javax.management.remote.rmi.RMIConnectorServer; import javax.naming.Context; import javax.rmi.ssl.SslRMIClientSocketFactory; -import lombok.extern.slf4j.Slf4j; import org.apache.hertzbeat.collector.collect.AbstractCollect; import org.apache.hertzbeat.collector.collect.common.cache.AbstractConnection; import org.apache.hertzbeat.collector.collect.common.cache.CacheIdentifier; @@ -54,13 +53,15 @@ import org.apache.hertzbeat.common.entity.job.Metrics; import org.apache.hertzbeat.common.entity.job.protocol.JmxProtocol; import org.apache.hertzbeat.common.entity.message.CollectRep; import org.apache.hertzbeat.common.util.CommonUtil; +import org.apache.hertzbeat.common.util.LogUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.util.Assert; import org.springframework.util.StringUtils; /** * jmx protocol acquisition implementation */ -@Slf4j public class JmxCollectImpl extends AbstractCollect { private static final String JMX_URL_PREFIX = "service:jmx:rmi:///jndi/rmi://"; @@ -75,6 +76,8 @@ public class JmxCollectImpl extends AbstractCollect { private final ClassLoader jmxClassLoader; + private static final Logger logger = LoggerFactory.getLogger(JmxCollectImpl.class); + public JmxCollectImpl() { jmxClassLoader = new JmxClassLoader(ClassLoader.getSystemClassLoader()); } @@ -195,12 +198,12 @@ public class JmxCollectImpl extends AbstractCollect { } } catch (IOException exception) { String errorMsg = CommonUtil.getMessageFromThrowable(exception); - log.error("JMX IOException :{}", errorMsg); + LogUtil.error(logger, "JMX IOException: {0}", errorMsg); builder.setCode(CollectRep.Code.UN_CONNECTABLE); builder.setMsg(errorMsg); } catch (Exception e) { String errorMsg = CommonUtil.getMessageFromThrowable(e); - log.error("JMX Error :{}", errorMsg); + LogUtil.error(logger, "JMX Error: {0}", errorMsg); builder.setCode(CollectRep.Code.FAIL); builder.setMsg(errorMsg); } finally { @@ -221,7 +224,7 @@ public class JmxCollectImpl extends AbstractCollect { for (Attribute attribute : attributeList.asList()) { Object value = attribute.getValue(); if (value == null) { - log.info("attribute {} value is null.", attribute.getName()); + LogUtil.info(logger, "attribute {0} value is null.", attribute.getName()); continue; } if (value instanceof Number || value instanceof String || value instanceof ObjectName @@ -245,7 +248,7 @@ public class JmxCollectImpl extends AbstractCollect { } attributeValueMap.put(attribute.getName(), builder.toString()); } else { - log.warn("attribute value type {} not support.", value.getClass().getName()); + LogUtil.warn(logger, "attribute value type {0} not support.", value.getClass().getName()); } } return attributeValueMap; @@ -319,7 +322,7 @@ public class JmxCollectImpl extends AbstractCollect { connectionCommonCache.addCache(identifier, new JmxConnect(conn)); return conn; } catch (Exception e) { - log.error("Failed to connect to JMX server: {}", e.getMessage()); + LogUtil.error(logger, "Failed to connect to JMX connection: {0}", e.getMessage()); throw new IOException("Failed to connect to JMX server: " + e.getMessage(), e); } } diff --git a/hertzbeat-common/src/main/java/org/apache/hertzbeat/common/util/LogUtil.java b/hertzbeat-common/src/main/java/org/apache/hertzbeat/common/util/LogUtil.java new file mode 100644 index 0000000000..a3255801a9 --- /dev/null +++ b/hertzbeat-common/src/main/java/org/apache/hertzbeat/common/util/LogUtil.java @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.hertzbeat.common.util; + +import org.apache.commons.lang3.ArrayUtils; +import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.builder.ToStringBuilder; +import org.apache.commons.lang3.builder.ToStringStyle; +import org.slf4j.Logger; + +import java.text.MessageFormat; + +/** + * Log utility class that provides formatted logging methods with location information. + * This class enhances standard SLF4J logging by automatically adding caller location details. + */ +public class LogUtil { + + private static final String TEMPLATE_REGEX = "\\{\\d}"; + + /** + * Print debug level formatted log + * Example: LogUtil.debug(logger, "hello,{0},here has a {1} exception", "other information"); + */ + @SuppressWarnings("unused") + public static void debug(Logger logger, String msg, Object... params) { + if (logger.isDebugEnabled()) { + + if (ArrayUtils.isEmpty(params)) { + logger.debug(LogUtil.buildLocationInfo() + msg); + } else { + logger.debug(LogUtil.buildLocationInfo() + format(msg, params)); + } + } + } + + + /** + * Print info level formatted log + * Example: LogUtil.info(logger, "hello,{0},{1} exception", "dear", "database operation"); + */ + public static void info(Logger logger, String msg, Object... params) { + if (logger.isInfoEnabled()) { + if (ArrayUtils.isEmpty(params)) { + logger.info(LogUtil.buildLocationInfo() + msg); + } else { + logger.info(LogUtil.buildLocationInfo() + format(msg, params)); + } + } + } + + /** + * Print warn level formatted log + */ + public static void warn(Logger logger, String msg, Object... params) { + if (logger.isWarnEnabled()) { + if (ArrayUtils.isEmpty(params)) { + logger.warn(LogUtil.buildLocationInfo() + msg); + } else { + logger.warn(LogUtil.buildLocationInfo() + format(msg, params)); + } + } + } + + /** + * Print error level formatted log, use {0},{1},.. for parameter replacement + * Example: LogUtil.error(logger, "hello,{0}, a {1} exception occurred here", "dear", "database operation"); + */ + public static void error(Logger logger, String msg, Object... params) { + if (logger.isErrorEnabled()) { + if (ArrayUtils.isEmpty(params)) { + logger.error(LogUtil.buildLocationInfo() + msg); + } else { + logger.error(LogUtil.buildLocationInfo() + format(msg, params)); + } + } + + } + + + /** + * Print warn level formatted log with exception, use {0},{1},.. for parameter replacement + * Example: LogUtil.warn(logger, e, "hello,{0}, a {1} exception occurred here", "dear", "database operation"); + */ + public static void warn(Logger logger, Throwable e, String msg, Object... params) { + if (logger.isWarnEnabled()) { + if (ArrayUtils.isEmpty(params)) { + logger.warn(LogUtil.buildLocationInfo() + msg, e); + } else { + logger.warn(LogUtil.buildLocationInfo() + format(msg, params), e); + } + } + } + + + /** + * Print error level formatted log with exception, use {0},{1},.. for parameter replacement + * Example: LogUtil.error(logger, e, "hello,{0}, a {1} exception occurred here", "dear", "database operation"); + */ + public static void error(Logger logger, Throwable e, String msg, Object... params) { + if (logger.isErrorEnabled()) { + if (ArrayUtils.isEmpty(params)) { + logger.error(LogUtil.buildLocationInfo() + msg, e); + } else { + logger.error(LogUtil.buildLocationInfo() + format(msg, params), e); + } + } + + } + + + /** + * Get the class name, method and line number that calls LogUtil + * + * @return location information string + */ + private static String buildLocationInfo() { + StringBuilder header = new StringBuilder(); + // LOG4J2-1029 new Throwable().getStackTrace is faster than Thread.currentThread().getStackTrace(). + final StackTraceElement[] stackTraceElements = new Throwable().getStackTrace(); + + for (int i = 0; i < stackTraceElements.length - 1; i++) { + StackTraceElement currentStackTrace = stackTraceElements[i]; + StackTraceElement nextStackTrace = stackTraceElements[i + 1]; + + // If current stack trace is in LogUtil + // and next stack trace is not in LogUtil + // then the next node is the caller of LogUtil + if (LogUtil.class.getName().equals(currentStackTrace.getClassName()) + && !LogUtil.class.getName().equals(nextStackTrace.getClassName())) { + String stackTrace = nextStackTrace.toString(); + header.append(" ").append(StringUtils.removeStart(stackTrace, nextStackTrace.getClassName() + ".")); + break; + } + } + return header.append(":").toString(); + } + + private static String format(String msg, Object... params) { + if (StringUtils.isEmpty(msg)) { + return StringUtils.EMPTY; + } + if (params != null && params.length > 0) { + msg = MessageFormat.format(msg, params); + } + return msg.replaceAll(TEMPLATE_REGEX, StringUtils.EMPTY); + } + + private static String toString(Object object) { + return ToStringBuilder.reflectionToString(object, ToStringStyle.SHORT_PREFIX_STYLE); + } +} \ No newline at end of file diff --git a/hertzbeat-common/src/test/java/org/apache/hertzbeat/common/util/LogUtilTest.java b/hertzbeat-common/src/test/java/org/apache/hertzbeat/common/util/LogUtilTest.java new file mode 100644 index 0000000000..176dbd3e2e --- /dev/null +++ b/hertzbeat-common/src/test/java/org/apache/hertzbeat/common/util/LogUtilTest.java @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.hertzbeat.common.util; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; +import org.slf4j.Logger; +import java.lang.reflect.Method; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.contains; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +class LogUtilTest { + + @Mock + private Logger mockLogger; + + private AutoCloseable mocks; + + @BeforeEach + void setUp() { + mocks = MockitoAnnotations.openMocks(this); + } + + @AfterEach + void tearDown() throws Exception { + if (mocks != null) { + mocks.close(); + } + } + + @Test + void testFormat_noParams_returnsOriginalMessage() throws Exception { + String original = "hello world"; + Method formatMethod = LogUtil.class.getDeclaredMethod("format", String.class, Object[].class); + formatMethod.setAccessible(true); + String formatted = (String) formatMethod.invoke(null, original, new Object[0]); + assertEquals(original, formatted); + } + + @Test + void testFormat_withParams_replacesPlaceholders() throws Exception { + String template = "hello,{0}, world {1}!"; + Method formatMethod = LogUtil.class.getDeclaredMethod("format", String.class, Object[].class); + formatMethod.setAccessible(true); + Object[] params = {"Alice", 123}; + String result = (String) formatMethod.invoke(null, template, params); + assertTrue(result.contains("hello,Alice")); + assertTrue(result.contains("world 123!")); + } + + @Test + void testDebug_noParams_logsRawMessage() { + when(mockLogger.isDebugEnabled()).thenReturn(true); + String msg = "test-debug"; + LogUtil.debug(mockLogger, msg); + verify(mockLogger).debug(contains(msg)); + } + + @Test + void testDebug_withParams_logsFormattedMessage() { + when(mockLogger.isDebugEnabled()).thenReturn(true); + LogUtil.debug(mockLogger, "user={0}", "Bob"); + verify(mockLogger).debug(contains("user=Bob")); + } + + @Test + void testInfo_levelOff_doesNotLog() { + when(mockLogger.isInfoEnabled()).thenReturn(false); + LogUtil.info(mockLogger, "should-not-log"); + verify(mockLogger, never()).info(anyString()); + } + + @Test + void testWarn_withException_logsMessageAndException() { + when(mockLogger.isWarnEnabled()).thenReturn(true); + RuntimeException ex = new RuntimeException("warn-ex"); + LogUtil.warn(mockLogger, ex, "warning {0}", "occurred"); + ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); + verify(mockLogger).warn(captor.capture(), eq(ex)); + assertTrue(captor.getValue().contains("warning occurred")); + } + + @Test + void testError_withExceptionAndParams_logsError() { + when(mockLogger.isErrorEnabled()).thenReturn(true); + RuntimeException ex = new RuntimeException("err"); + LogUtil.error(mockLogger, ex, "fail code {0}", 500); + ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); + verify(mockLogger).error(captor.capture(), eq(ex)); + assertTrue(captor.getValue().contains("fail code 500")); + } +}