chore(bqjdbc): refactor log levels#13166
Conversation
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.
| if (LOG.isLoggable(Level.FINER)) { | ||
| LOG.logp( | ||
| Level.FINER, | ||
| target.getClass().getName(), | ||
| method.getName(), | ||
| () -> | ||
| String.format( | ||
| "Entering: %s.%s(%s)", | ||
| interfaceType.getSimpleName(), | ||
| method.getName(), | ||
| args != null ? java.util.Arrays.toString(args) : "")); | ||
| } |
There was a problem hiding this comment.
Using java.util.Arrays.toString(args) within the log message supplier poses a reliability risk. If any argument's toString() implementation throws an exception, it will cause the entire JDBC method invocation to fail. Since this proxy wraps most JDBC operations, a buggy toString() in a user-provided object (e.g., passed via setObject) could lead to application crashes when FINER logging is enabled. It is safer to use a utility that handles toString() exceptions gracefully or to avoid stringifying complex objects directly in the proxy.
| if (LOG.isLoggable(Level.FINER) | ||
| && !(java.sql.Connection.class.isAssignableFrom(interfaceType) | ||
| && "close".equals(method.getName()))) { | ||
| LOG.logp( | ||
| Level.FINER, | ||
| target.getClass().getName(), | ||
| method.getName(), | ||
| () -> | ||
| String.format( | ||
| "Exiting: %s.%s() -> %s", | ||
| interfaceType.getSimpleName(), | ||
| method.getName(), | ||
| result != null ? result.toString() : "null")); | ||
| } |
There was a problem hiding this comment.
Calling result.toString() here is risky if the result's toString() method throws an exception. Additionally, in JDBC dynamic proxy implementations, avoid wrapping or performing heavy operations on java.sql.ResultSet if performance on iteration hot-paths is critical. Bypassing proxies for ResultSet prevents overhead from dynamic invocation and argument allocations, enabling the JIT compiler to inline getters. If this is an intentional omission for performance, ensure it is documented.
References
- In JDBC dynamic proxy implementations, avoid wrapping java.sql.ResultSet if performance on iteration hot-paths is critical to allow JIT inlining.
| new LogRecord(Level.FINER, String.format("New thread %s created.", t.getName())); | ||
| record.setSourceClassName(BigQueryThreadFactory.class.getName()); | ||
| record.setSourceMethodName("newThread"); | ||
| record.setThreadID((int) t.getId()); |
There was a problem hiding this comment.
The LogRecord thread ID should represent the thread that is generating the log message (the current thread), not the thread being created. Setting it to t.getId() is misleading as it suggests the log was produced by the new thread, which hasn't started yet. Furthermore, if LOG is an instance of BigQueryJdbcResultSetLogger, this value will be overwritten by the current thread's ID in its log(LogRecord) override, making this line redundant.
| record.setThreadID((int) t.getId()); | |
| // Thread ID is automatically handled by the logger |
No description provided.