chore(bqjdbc): refactor log levels#13166
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the logging infrastructure of the BigQuery JDBC driver to improve performance and consistency. It removes numerous manual method entry/exit logs across the codebase, replacing them with a centralized logging mechanism within BigQueryJdbcContextProxy using dynamic proxies. The BigQueryJdbcResultSetLogger and BigQueryJdbcRootLogger were updated to better handle LogRecord objects and thread IDs. Feedback highlights potential reliability risks in the proxy's logging logic where calling toString() on arguments or results could trigger exceptions, and notes that setting thread IDs manually in the thread factory is misleading and redundant.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the logging infrastructure across the BigQuery JDBC driver, primarily shifting method entry/exit tracing from Level.FINEST to Level.FINER and introducing a more structured LogRecord approach to avoid expensive stack trace parsing. It also enhances the BigQueryJdbcContextProxy to automatically log method calls and results. Feedback focuses on a security concern regarding the potential for sensitive data exposure (CWE-532) when logging all method arguments and return values at the FINER level, as well as the performance implications of wrapping high-frequency JDBC operations.
| } | ||
|
|
||
| void append(JsonArray data, long offset) throws DescriptorValidationException, IOException { | ||
| if (LOG.isLoggable(Level.FINER)) { |
There was a problem hiding this comment.
nit: no need to check this, it constructs MsgSupplier object under the cover, so no real string.format
There was a problem hiding this comment.
I will keep this because appends is a hotpath when inserting with BulkInsertWriter. The check helps eliminate the hidden cost of varargs allocation, auto-boxing and lambda creation.
| } | ||
|
|
||
| public void onFailure(Throwable throwable) { | ||
| parent.LOG.warning( |
There was a problem hiding this comment.
Why using parent.LOG there and LOG in other places?
There was a problem hiding this comment.
The AppendCompleteCallback class is static and the LOG is not. Hence the parent.LOG reference.
| "\n" + Thread.currentThread().getName() + " Error @ arrowStreamProcessor", | ||
| e); | ||
| enqueueError(arrowBatchWrapperBlockingQueue, e); | ||
| if (e.getCause() instanceof InterruptedException |
There was a problem hiding this comment.
Use a separate catch (InterruptedException) {} instead
There was a problem hiding this comment.
This was added to distinguish between actual errors/interruption to the worker thread operation vs shutdown signal send by the jdbc driver. Since standard Java Iterator and stream interfaces do not allow checked exceptions in their signatures, any underlying InterruptedException is typically wrapped inside an unchecked exception (such as RuntimeException, UncheckedExecutionException, or GAX's ApiException). Catching InterruptedException directly will miss these wrapped occurrences.
| "\n" + Thread.currentThread().getName() + " Error @ populateBufferAsync", | ||
| ex); | ||
| enqueueBufferError(bigQueryFieldValueListWrapperBlockingQueue, ex); | ||
| if (ex.getCause() instanceof InterruptedException |
There was a problem hiding this comment.
same, separate catch clause
| public void finestTrace(String methodName, String msg) { | ||
| if (isLoggable(Level.FINEST)) { | ||
| logp(Level.FINEST, targetClassName, methodName, msg); | ||
| LogRecord record = new LogRecord(Level.FINEST, msg); |
There was a problem hiding this comment.
Should this be done as a part of log(record) ?
There was a problem hiding this comment.
The standard log(LogRecord record) method signature in Java's java.util.logging.Logger framework requires accepting an already-constructed LogRecord object as its argument. So I moved the LogRecord creation to private helper methods instead.
|
|
||
| Object result = method.invoke(target, args); | ||
|
|
||
| // Suppress exit logging for Connection.close() since its file handler is unmounted during |
There was a problem hiding this comment.
Do we need this? I'd guess if file handler is removed at this point, nothing will be logged either way
There was a problem hiding this comment.
The file handler for the connection is removed at this point, so without the check this particular close log gets logged to the global file handler.
2 participants
b/505825091
- Causal Exception Unwrapping: Updated async stream readers (BigQueryStatement) to check e.getCause(), properly categorizing wrapped network interrupts during query shutdown to prevent false severe error logs.
- POJO Log Stripping: Removed repetitive, low-level diagnostic logs from intermediate state wrappers (BigQueryCallableStatement) to improve per-connection file readability.
- Standardized Level Routing: All Non-ResultSet trace logs now logs at Level.FINER, cleanly separating infrastructure execution boundaries from raw data iteration outputs (Level.FINEST).