Skip to content

Add logging abstraction with SLF4J backend#740

Open
mihaimitrea-db wants to merge 1 commit intomainfrom
mihaimitrea-db/stack/logging-abstraction
Open

Add logging abstraction with SLF4J backend#740
mihaimitrea-db wants to merge 1 commit intomainfrom
mihaimitrea-db/stack/logging-abstraction

Conversation

@mihaimitrea-db
Copy link
Copy Markdown
Contributor

@mihaimitrea-db mihaimitrea-db commented Mar 26, 2026

🥞 Stacked PR

Use this link to review incremental changes.


Summary

Introduces a logging abstraction layer (com.databricks.sdk.core.logging) that decouples the SDK's internal logging from any specific backend. SLF4J remains the default. This PR contains only the abstraction and the SLF4J backend; the JUL backend and the call-site migration follow in stacked PRs.

Why

The SDK currently imports org.slf4j.Logger and org.slf4j.LoggerFactory directly in every class that logs. This hard coupling means users who embed the SDK in environments where SLF4J is impractical (e.g. BI tools with constrained classpaths) have no way to switch to an alternative logging backend like java.util.logging.

We need an indirection layer that lets users swap the logging backend programmatically while keeping SLF4J as the zero-configuration default so that existing users don't have to change anything.

The design follows the pattern established by Netty's InternalLoggerFactory: an abstract LoggerFactory class with concrete subclasses for each backend, a setDefault method to override the backend, and a separate Logger abstract class that serves as a clean extension point for custom implementations.

What changed

Interface changes

  • Logger — new abstract class in com.databricks.sdk.core.logging. Defines the logging contract: debug, info, warn, error (each with plain-string and varargs overloads), and isDebugEnabled. Users extend this to build custom loggers.
  • LoggerFactory — new abstract class. Static methods getLogger(Class<?>) and getLogger(String) return loggers from the current default factory. setDefault(LoggerFactory) overrides the backend — must be called before creating any SDK client.
  • Slf4jLoggerFactory — public concrete factory with a singleton INSTANCE. This is the default.

Behavioral changes

None. SLF4J is the default backend and all logging calls pass through to org.slf4j.Logger exactly as before. Existing users see no difference.

Internal changes

  • Slf4jLogger — package-private class that delegates all calls to an org.slf4j.Logger. Fully qualifies org.slf4j.LoggerFactory references to avoid collision with the SDK's LoggerFactory.
  • All new classes live in com.databricks.sdk.core.logging.

How is this tested?

  • LoggerFactoryTest — verifies the default factory is SLF4J, that setDefault(null) is rejected, and that getLogger(String) works.
  • Slf4jLoggerTest — verifies Slf4jLogger.create returns the correct type, and exercises all logging methods including varargs and trailing Throwable.
  • Full test suite (1140 tests) passes.

@mihaimitrea-db mihaimitrea-db self-assigned this Mar 26, 2026
@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/logging-abstraction branch from 9ac10fc to 99b473a Compare March 27, 2026 09:19
@mihaimitrea-db
Copy link
Copy Markdown
Contributor Author

Range-diff: main (9ac10fc -> 99b473a)
databricks-sdk-java/src/main/java/com/databricks/sdk/core/logging/LoggerFactory.java
@@ -4,6 +4,8 @@
 +++ b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/logging/LoggerFactory.java
 +package com.databricks.sdk.core.logging;
 +
++import java.util.concurrent.atomic.AtomicReference;
++
 +/**
 + * Creates and configures {@link Logger} instances for the SDK.
 + *
@@ -19,7 +21,7 @@
 + */
 +public abstract class LoggerFactory {
 +
-+  private static volatile LoggerFactory defaultFactory = Slf4jLoggerFactory.INSTANCE;
++  private static final AtomicReference<LoggerFactory> defaultFactory = new AtomicReference<>();
 +
 +  /** Returns a logger for the given class, using the current default factory. */
 +  public static Logger getLogger(Class<?> type) {
@@ -41,11 +43,16 @@
 +    if (factory == null) {
 +      throw new IllegalArgumentException("LoggerFactory must not be null");
 +    }
-+    defaultFactory = factory;
++    defaultFactory.set(factory);
 +  }
 +
 +  static LoggerFactory getDefault() {
-+    return defaultFactory;
++    LoggerFactory f = defaultFactory.get();
++    if (f != null) {
++      return f;
++    }
++    defaultFactory.compareAndSet(null, Slf4jLoggerFactory.INSTANCE);
++    return defaultFactory.get();
 +  }
 +
 +  /** Creates a new logger for the given class. */

Reproduce locally: git range-diff 29672b6..9ac10fc 29672b6..99b473a | Disable: git config gitstack.push-range-diff false

*/
public abstract class Logger {

public abstract boolean isDebugEnabled();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How and why is this function used?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As of right now this function is only used in ApiClient.java:L256. It's used to guard against a call to LOG.debug where constructing the message is relatively expensive and runs at every request:

response = httpClient.execute(in);
if (LOG.isDebugEnabled()) {
     LOG.debug(makeLogRecord(in, response));
}

Users can customize their logger's level of detail in the runtime config. If they are not interested in debug-level details it makes sense to not slow down every request with constructing the log. This behaviour is not new to this PR and I think it makes sense to keep it.

This check could be made as a guard in the debug function itself to abstract over this, but both JUL (.isLoggable(Level.FINE)) and SLF4J (.isDebugEnabled()) implement some sort of way of checking if you want to log for debugs.

Would you prefer something else?


/** Returns a logger for the given class, using the current default factory. */
public static Logger getLogger(Class<?> type) {
return getDefault().newInstance(type);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does newInstance always create a new logger? What will happen when a logger exists for a given class?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With the wrapper we are introducing you will get a new instance of the desired logger wrapper. However behind the scenes for SLF4J and JUL keep track of existing loggers and see if you have already instantiated the requested logger before. If you have the .getLogger() function will provide you with the same instance.

As per the SLF4J FAQ:

More specifically, each time a logger is retrieved by invoking LoggerFactory.getLogger() method, the underlying logging system will return an instance appropriate for the current application. Please note that within the same application retrieving a logger by a given name will always return the same logger. For a given name, a different logger will be returned only for different applications.

The only difference in our case is that we are creating a new wrapper instance. In practice this should NOT happen because every class except for CallbackResponseHandler and NonIdempotentRequestRetryStrategy use the static keyword when initializing their logger. These two classes follow a different method of initializing their loggers, namely: .getLogger(getClass().getName()).

@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/logging-abstraction branch from 99b473a to d550d5e Compare March 30, 2026 08:16
@mihaimitrea-db
Copy link
Copy Markdown
Contributor Author

Range-diff: main (99b473a -> d550d5e)
databricks-sdk-java/src/main/java/com/databricks/sdk/core/logging/Slf4jLoggerFactory.java
@@ -4,7 +4,7 @@
 +++ b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/logging/Slf4jLoggerFactory.java
 +package com.databricks.sdk.core.logging;
 +
-+/** A {@link LoggerFactory} backed by SLF4J. This is the default. */
++/** A {@link LoggerFactory} backed by SLF4J. */
 +public class Slf4jLoggerFactory extends LoggerFactory {
 +
 +  public static final Slf4jLoggerFactory INSTANCE = new Slf4jLoggerFactory();
databricks-sdk-java/src/test/java/com/databricks/sdk/core/logging/Slf4jLoggerTest.java
@@ -6,7 +6,15 @@
 +
 +import static org.junit.jupiter.api.Assertions.*;
 +
++import java.util.ArrayList;
++import java.util.List;
++import java.util.stream.Stream;
++import org.apache.log4j.AppenderSkeleton;
++import org.apache.log4j.spi.LoggingEvent;
 +import org.junit.jupiter.api.Test;
++import org.junit.jupiter.params.ParameterizedTest;
++import org.junit.jupiter.params.provider.Arguments;
++import org.junit.jupiter.params.provider.MethodSource;
 +
 +public class Slf4jLoggerTest {
 +
@@ -18,18 +26,105 @@
 +  }
 +
 +  @Test
-+  void slf4jLoggerOperations() {
++  void isDebugEnabledReflectsBackend() {
++    // com.databricks is set to TRACE in log4j.properties
 +    Logger logger = Slf4jLogger.create(Slf4jLoggerTest.class);
-+    logger.debug("debug");
-+    logger.debug("debug {}", "arg");
-+    logger.info("info");
-+    logger.info("info {}", "arg");
-+    logger.warn("warn");
-+    logger.warn("warn {}", "arg");
-+    logger.error("error");
-+    logger.error("error {}", "arg");
-+    logger.error("error {} failed", "op", new RuntimeException("cause"));
-+    logger.isDebugEnabled();
++    assertTrue(logger.isDebugEnabled());
 +  }
 +
++  static Stream<Arguments> logCalls() {
++    RuntimeException ex = new RuntimeException("boom");
++    return Stream.of(
++        Arguments.of("debug", "hello", null, "hello", null),
++        Arguments.of("info", "hello", null, "hello", null),
++        Arguments.of("warn", "hello", null, "hello", null),
++        Arguments.of("error", "hello", null, "hello", null),
++        Arguments.of("info", "user {} logged in", new Object[] {"alice"}, "user alice logged in", null),
++        Arguments.of("info", "a={}, b={}", new Object[] {1, 2}, "a=1, b=2", null),
++        Arguments.of("error", "failed: {}", new Object[] {"op", ex}, "failed: op", ex),
++        Arguments.of("error", "Error: {}", new Object[] {ex}, "Error: {}", ex),
++        Arguments.of("error", "Something broke", new Object[] {ex}, "Something broke", ex));
++  }
++
++  @ParameterizedTest(name = "[{index}] {0}(\"{1}\")")
++  @MethodSource("logCalls")
++  void deliversCorrectOutput(
++      String level, String format, Object[] args, String expectedMsg, Throwable expectedThrown) {
++    CapturingAppender appender = new CapturingAppender();
++    org.apache.log4j.Logger log4jLogger =
++        org.apache.log4j.Logger.getLogger(Slf4jLoggerTest.class);
++    log4jLogger.addAppender(appender);
++    try {
++      Logger logger = Slf4jLogger.create(Slf4jLoggerTest.class);
++      dispatch(logger, level, format, args);
++
++      assertEquals(1, appender.events.size(), "Expected exactly one log event");
++      LoggingEvent event = appender.events.get(0);
++      assertEquals(expectedMsg, event.getRenderedMessage());
++      assertEquals(toLog4jLevel(level), event.getLevel());
++      if (expectedThrown != null) {
++        assertNotNull(event.getThrowableInformation(), "Expected throwable to be attached");
++        assertSame(expectedThrown, event.getThrowableInformation().getThrowable());
++      } else {
++        assertNull(event.getThrowableInformation(), "Expected no throwable");
++      }
++    } finally {
++      log4jLogger.removeAppender(appender);
++    }
++  }
++
++  private static void dispatch(Logger logger, String level, String format, Object[] args) {
++    switch (level) {
++      case "debug":
++        if (args != null) logger.debug(format, args);
++        else logger.debug(format);
++        break;
++      case "info":
++        if (args != null) logger.info(format, args);
++        else logger.info(format);
++        break;
++      case "warn":
++        if (args != null) logger.warn(format, args);
++        else logger.warn(format);
++        break;
++      case "error":
++        if (args != null) logger.error(format, args);
++        else logger.error(format);
++        break;
++      default:
++        throw new IllegalArgumentException("Unknown level: " + level);
++    }
++  }
++
++  private static org.apache.log4j.Level toLog4jLevel(String level) {
++    switch (level) {
++      case "debug":
++        return org.apache.log4j.Level.DEBUG;
++      case "info":
++        return org.apache.log4j.Level.INFO;
++      case "warn":
++        return org.apache.log4j.Level.WARN;
++      case "error":
++        return org.apache.log4j.Level.ERROR;
++      default:
++        throw new IllegalArgumentException("Unknown level: " + level);
++    }
++  }
++
++  static class CapturingAppender extends AppenderSkeleton {
++    final List<LoggingEvent> events = new ArrayList<>();
++
++    @Override
++    protected void append(LoggingEvent event) {
++      events.add(event);
++    }
++
++    @Override
++    public void close() {}
++
++    @Override
++    public boolean requiresLayout() {
++      return false;
++    }
++  }
 +}
\ No newline at end of file

Reproduce locally: git range-diff 29672b6..99b473a 29672b6..d550d5e | Disable: git config gitstack.push-range-diff false

@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/logging-abstraction branch from d550d5e to 9597522 Compare March 30, 2026 08:33
@mihaimitrea-db
Copy link
Copy Markdown
Contributor Author

Range-diff: main (d550d5e -> 9597522)
NEXT_CHANGELOG.md
@@ -0,0 +1,10 @@
+diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md
+--- a/NEXT_CHANGELOG.md
++++ b/NEXT_CHANGELOG.md
+ ### Documentation
+ 
+ ### Internal Changes
++* Introduced a logging abstraction (`com.databricks.sdk.core.logging`) that decouples the SDK from SLF4J. Users can now provide their own logging backend by extending `LoggerFactory` and calling `LoggerFactory.setDefault()` before creating any SDK client. SLF4J remains the default.
+ 
+ ### API Changes
+ * Add `createCatalog()`, `createSyncedTable()`, `deleteCatalog()`, `deleteSyncedTable()`, `getCatalog()` and `getSyncedTable()` methods for `workspaceClient.postgres()` service.
\ No newline at end of file
databricks-sdk-java/src/main/java/com/databricks/sdk/core/logging/LoggerFactory.java
@@ -9,8 +9,8 @@
 +/**
 + * Creates and configures {@link Logger} instances for the SDK.
 + *
-+ * <p>By default, logging goes through SLF4J. Users can override the backend programmatically
-+ * before creating any SDK client:
++ * <p>By default, logging goes through SLF4J. Users can override the backend programmatically before
++ * creating any SDK client:
 + *
 + * <pre>{@code
 + * LoggerFactory.setDefault(myCustomFactory);
@@ -36,8 +36,8 @@
 +  /**
 +   * Overrides the logging backend used by the SDK.
 +   *
-+   * <p>Must be called before creating any SDK client or calling {@link #getLogger}. Loggers
-+   * already obtained will not be affected by subsequent calls.
++   * <p>Must be called before creating any SDK client or calling {@link #getLogger}. Loggers already
++   * obtained will not be affected by subsequent calls.
 +   */
 +  public static void setDefault(LoggerFactory factory) {
 +    if (factory == null) {
databricks-sdk-java/src/test/java/com/databricks/sdk/core/logging/Slf4jLoggerTest.java
@@ -39,7 +39,8 @@
 +        Arguments.of("info", "hello", null, "hello", null),
 +        Arguments.of("warn", "hello", null, "hello", null),
 +        Arguments.of("error", "hello", null, "hello", null),
-+        Arguments.of("info", "user {} logged in", new Object[] {"alice"}, "user alice logged in", null),
++        Arguments.of(
++            "info", "user {} logged in", new Object[] {"alice"}, "user alice logged in", null),
 +        Arguments.of("info", "a={}, b={}", new Object[] {1, 2}, "a=1, b=2", null),
 +        Arguments.of("error", "failed: {}", new Object[] {"op", ex}, "failed: op", ex),
 +        Arguments.of("error", "Error: {}", new Object[] {ex}, "Error: {}", ex),
@@ -51,8 +52,7 @@
 +  void deliversCorrectOutput(
 +      String level, String format, Object[] args, String expectedMsg, Throwable expectedThrown) {
 +    CapturingAppender appender = new CapturingAppender();
-+    org.apache.log4j.Logger log4jLogger =
-+        org.apache.log4j.Logger.getLogger(Slf4jLoggerTest.class);
++    org.apache.log4j.Logger log4jLogger = org.apache.log4j.Logger.getLogger(Slf4jLoggerTest.class);
 +    log4jLogger.addAppender(appender);
 +    try {
 +      Logger logger = Slf4jLogger.create(Slf4jLoggerTest.class);

Reproduce locally: git range-diff 29672b6..d550d5e 29672b6..9597522 | Disable: git config gitstack.push-range-diff false

@github-actions
Copy link
Copy Markdown

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/sdk-java

Inputs:

  • PR number: 740
  • Commit SHA: 9597522bab3dd1a6277b714b462fb58926307300

Checks will be approved automatically on success.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants