fix(bigquery-jdbc): feature branch failing tests#13287
Conversation
There was a problem hiding this comment.
Code Review
This pull request integrates OpenTelemetry tracing and logging into the BigQuery JDBC driver, adding a custom JUL logging handler (OpenTelemetryJulHandler) and updating core connection, statement, and metadata classes to support tracing and context propagation. The review feedback identifies a performance risk from synchronous blocking network calls inside the logging handler's publish method, recommending offloading to a bounded thread pool. Additionally, it highlights a thread-safety issue in ensureGlobalHandlerAttached that could lead to duplicate handlers, suggesting the use of an explicit lock to secure concurrent initialization.
I am having trouble creating individual review comments. Click here to see my feedback.
java-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/OpenTelemetryJulHandler.java (110)
Calling loggingClient.write synchronously inside the publish method of a logging Handler introduces a blocking network call on the application's critical path. This can severely degrade performance, especially when logging is verbose (e.g., at FINE or INFO levels). Consider offloading the log writing to a background bounded thread pool (e.g., ThreadPoolExecutor with a defined maximum size) or queue, or configuring the Logging client with asynchronous batching options.
References
- For safety, use a bounded thread pool (e.g.,
ThreadPoolExecutorwith a defined maximum size) instead of an unbounded one (e.g.,Executors.newCachedThreadPool()), even if the current logic seems to limit concurrent tasks.
java-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcOpenTelemetry.java (143-155)
The ensureGlobalHandlerAttached method is not thread-safe. If multiple connections are initialized concurrently, multiple threads might check logger.getHandlers() simultaneously, find no handler, and both add a new OpenTelemetryJulHandler. This would result in duplicate handlers and duplicate log exports. To protect this shared state while ensuring thread safety and visibility in performance-sensitive code, prefer using an explicit lock over the synchronized keyword.
private static final java.util.concurrent.locks.ReentrantLock lock = new java.util.concurrent.locks.ReentrantLock();
public static void ensureGlobalHandlerAttached() {
lock.lock();
try {
Logger logger = Logger.getLogger(BIGQUERY_NAMESPACE);
boolean present = false;
for (Handler h : logger.getHandlers()) {
if (h instanceof OpenTelemetryJulHandler) {
present = true;
break;
}
}
if (!present) {
logger.addHandler(new OpenTelemetryJulHandler());
}
} finally {
lock.unlock();
}
}References
- In performance-sensitive code, prefer using explicit locks over the 'synchronized' keyword to protect shared state while ensuring thread safety and visibility.
b/517498094
This PR fixes dependency analysis failures and flaky test issues identified in the OpenTelemetry integration feature branch.