Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,34 @@
package com.google.api.gax.logging;

import com.google.api.core.InternalApi;
import com.google.common.annotations.VisibleForTesting;
import java.util.Map;

@InternalApi
public class LoggingUtils {

private static boolean loggingEnabled = isLoggingEnabled();
private static boolean loggingEnabled = checkLoggingEnabled();
static final String GOOGLE_SDK_JAVA_LOGGING = "GOOGLE_SDK_JAVA_LOGGING";

static boolean isLoggingEnabled() {
/**
* Returns whether client-side logging is enabled.
*
* @return true if logging is enabled, false otherwise.
*/
public static boolean isLoggingEnabled() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this method remain package private?

return loggingEnabled;
}

/**
* Sets whether client-side logging is enabled. Visible for testing.
*
* @param enabled true to enable logging, false to disable.
*/
public static void setLoggingEnabled(boolean enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be package private and annotated with a @VisibleForTesting.

loggingEnabled = enabled;
}

private static boolean checkLoggingEnabled() {
String enableLogging = System.getenv(GOOGLE_SDK_JAVA_LOGGING);
return "true".equalsIgnoreCase(enableLogging);
}
Expand Down Expand Up @@ -126,6 +145,27 @@ public static <RespT> void logRequest(
}
}

/**
* Logs an actionable error message with structured context.
*
* @param logContext A map containing the structured logging context (e.g., RPC service, method,
* error details).
* @param loggerProvider The provider used to obtain the logger.
* @param message The human-readable error message.
*/
public static void logActionableError(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

According to the repository's testing guidelines, new logic in the gax module must be accompanied by unit tests. This new public method logActionableError should have corresponding tests in LoggingUtilsTest to ensure its correctness and prevent future regressions.

References
  1. The repository style guide (line 75) mandates that changes in the gax module must include traditional unit tests. (link)

Map<String, Object> logContext, LoggerProvider loggerProvider, String message) {
if (loggingEnabled) {
org.slf4j.Logger logger = loggerProvider.getLogger();
// Actionable errors are logged at the INFO level because transport errors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable errors are logged at the INFO level

Do we have a cross-language requirements for this?

// might be retryable and self-healing. Logging at ERROR would trigger
// unintended production alerts for transient issues.
if (logger.isInfoEnabled()) {
Slf4jUtils.log(logger, org.slf4j.event.Level.INFO, logContext, message);
}
}
}
Comment on lines +156 to +167
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The method logActionableError logs messages at the INFO level. Given the name of the method, it would be more appropriate to log these messages at a WARN or ERROR level. Logging actionable errors as INFO might cause them to be overlooked in production environments where log levels are filtered. Consider using the WARN level for these messages.

Suggested change
public static void logActionableError(
Map<String, Object> logContext, LoggerProvider loggerProvider, String message) {
if (loggingEnabled) {
org.slf4j.Logger logger = loggerProvider.getLogger();
if (logger.isInfoEnabled()) {
Slf4jUtils.log(logger, org.slf4j.event.Level.INFO, logContext, message);
}
}
}
public static void logActionableError(
Map<String, Object> logContext, LoggerProvider loggerProvider, String message) {
if (loggingEnabled) {
org.slf4j.Logger logger = loggerProvider.getLogger();
if (logger.isWarnEnabled()) {
Slf4jUtils.log(logger, org.slf4j.event.Level.WARN, logContext, message);
}
}
}


public static void executeWithTryCatch(ThrowingRunnable action) {
try {
action.run();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,20 @@
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import com.google.api.gax.logging.LoggingUtils.ThrowingRunnable;
import java.util.Collections;
import java.util.Map;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;
import org.slf4j.Logger;

class LoggingUtilsTest {

Expand Down Expand Up @@ -77,4 +86,53 @@ void testExecuteWithTryCatch_WithNoSuchMethodError() throws Throwable {
// Verify that the action was executed (despite the error)
verify(action).run();
}

@AfterEach
void tearDown() {
LoggingUtils.setLoggingEnabled(false);
}

@Test
void testLogActionableError_loggingDisabled() {
LoggingUtils.setLoggingEnabled(false);
LoggerProvider loggerProvider = mock(LoggerProvider.class);

LoggingUtils.logActionableError(Collections.emptyMap(), loggerProvider, "message");

verify(loggerProvider, never()).getLogger();
}

@Test
void testLogActionableError_infoDisabled() {
LoggingUtils.setLoggingEnabled(true);
LoggerProvider loggerProvider = mock(LoggerProvider.class);
Logger logger = mock(Logger.class);
when(loggerProvider.getLogger()).thenReturn(logger);
when(logger.isInfoEnabled()).thenReturn(false);

LoggingUtils.logActionableError(Collections.emptyMap(), loggerProvider, "message");

verify(loggerProvider).getLogger();
verify(logger).isInfoEnabled();
verify(logger, never()).info(anyString());
}

@Test
void testLogActionableError_success() {
LoggingUtils.setLoggingEnabled(true);
LoggerProvider loggerProvider = mock(LoggerProvider.class);
Logger logger = mock(Logger.class);
when(loggerProvider.getLogger()).thenReturn(logger);
when(logger.isInfoEnabled()).thenReturn(true);

org.slf4j.spi.LoggingEventBuilder eventBuilder = mock(org.slf4j.spi.LoggingEventBuilder.class);
when(logger.atInfo()).thenReturn(eventBuilder);
when(eventBuilder.addKeyValue(anyString(), any())).thenReturn(eventBuilder);

Map<String, Object> context = Collections.singletonMap("key", "value");
LoggingUtils.logActionableError(context, loggerProvider, "message");

verify(loggerProvider).getLogger();
verify(logger).isInfoEnabled();
}
}
Loading