Skip to content

feat(bqjdbc): implement Per connection logging with Context proxy#13001

Open
Neenu1995 wants to merge 13 commits intomainfrom
ns/per-con-log-alt
Open

feat(bqjdbc): implement Per connection logging with Context proxy#13001
Neenu1995 wants to merge 13 commits intomainfrom
ns/per-con-log-alt

Conversation

@Neenu1995
Copy link
Copy Markdown
Contributor

@Neenu1995 Neenu1995 commented May 5, 2026

b/501081433

  1. Foundational MDC & Global File Handler Routing
    Simplifies the BigQueryJdbcMdc ThreadLocal context slots, sets up "Jdbc-global" fallback file routing, and adds isFileLoggingEnabled() to the root logger.

  2. Dynamic Context Proxy Wrapper & Connection Wrap
    Implements the BigQueryJdbcContextProxy dynamic proxy interceptor, delegating standard wrappers, and wrapping all new JDBC entries in BigQueryDriver.

  3. Statement Modifications & Connection Pool unwrapping
    Adapts Statements and Connection ID builders to utilize the dynamic proxy, and updates connection pool managers to safely extract raw connection properties via standard unwrap calls.

  4. 0%-Overhead Raw ResultSet & Metadata Routing
    Completely un-proxies ResultSet to achieve 0% query execution overhead, while implementing targeted ResultSetMetaData proxy wrapping inside getMetaData() to keep metadata queries connection-routed.

  5. Decoupled Exceptions & Clean Integration Tests
    Decouples all custom exception constructors to eliminate double-logging, converts all 17 integration test class casts to standard unwrap calls, and migrates all deprecated github_timeline public dataset queries to the active shakespeare dataset.

@Neenu1995 Neenu1995 requested review from a team as code owners May 5, 2026 01:33
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a dynamic proxy mechanism, BigQueryJdbcContextProxy, to automate the management of connection-specific logging contexts (MDC) and unify exception logging across JDBC components. It simplifies the BigQueryJdbcMdc implementation and refactors various classes to remove manual logging overhead. Review feedback suggests extending the proxy to ResultSet to maintain context during data retrieval, handling Object methods within the proxy, correcting the isWrapperFor logic, and improving null-safety in exception message logging.

Comment on lines +88 to +100
if (result instanceof java.sql.CallableStatement) {
return wrap(result, java.sql.CallableStatement.class, connectionId);
} else if (result instanceof java.sql.PreparedStatement) {
return wrap(result, java.sql.PreparedStatement.class, connectionId);
} else if (result instanceof java.sql.Statement) {
return wrap(result, java.sql.Statement.class, connectionId);
} else if (result instanceof java.sql.DatabaseMetaData) {
return wrap(result, java.sql.DatabaseMetaData.class, connectionId);
} else if (result instanceof java.sql.ParameterMetaData) {
return wrap(result, java.sql.ParameterMetaData.class, connectionId);
} else if (result instanceof java.sql.ResultSetMetaData) {
return wrap(result, java.sql.ResultSetMetaData.class, connectionId);
}
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.

high

The proxy should also cascade to java.sql.ResultSet. Without wrapping the ResultSet, operations like next() or getString() will execute without the connection context in the MDC, which defeats the purpose of per-connection logging for the most common JDBC operations. If performance on the result set hot-path is a concern, consider that Statement and Connection are already being proxied.

Suggested change
if (result instanceof java.sql.CallableStatement) {
return wrap(result, java.sql.CallableStatement.class, connectionId);
} else if (result instanceof java.sql.PreparedStatement) {
return wrap(result, java.sql.PreparedStatement.class, connectionId);
} else if (result instanceof java.sql.Statement) {
return wrap(result, java.sql.Statement.class, connectionId);
} else if (result instanceof java.sql.DatabaseMetaData) {
return wrap(result, java.sql.DatabaseMetaData.class, connectionId);
} else if (result instanceof java.sql.ParameterMetaData) {
return wrap(result, java.sql.ParameterMetaData.class, connectionId);
} else if (result instanceof java.sql.ResultSetMetaData) {
return wrap(result, java.sql.ResultSetMetaData.class, connectionId);
}
if (result instanceof java.sql.CallableStatement) {
return wrap(result, java.sql.CallableStatement.class, connectionId);
} else if (result instanceof java.sql.PreparedStatement) {
return wrap(result, java.sql.PreparedStatement.class, connectionId);
} else if (result instanceof java.sql.Statement) {
return wrap(result, java.sql.Statement.class, connectionId);
} else if (result instanceof java.sql.ResultSet) {
return wrap(result, java.sql.ResultSet.class, connectionId);
} else if (result instanceof java.sql.DatabaseMetaData) {
return wrap(result, java.sql.DatabaseMetaData.class, connectionId);
} else if (result instanceof java.sql.ParameterMetaData) {
return wrap(result, java.sql.ParameterMetaData.class, connectionId);
} else if (result instanceof java.sql.ResultSetMetaData) {
return wrap(result, java.sql.ResultSetMetaData.class, connectionId);
}

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.

This is intentional.

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.

Can you add a comment about it?

}
}

@Override
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.

Why adding these overrides here?

When we talked earlier, I meant for JdbcRootLogger() we will need to remove (throw an exception) when finest/finer is used & adding another logger JdbcResultSetLogger with finer/finest levels.

Copy link
Copy Markdown
Contributor Author

@Neenu1995 Neenu1995 May 5, 2026

Choose a reason for hiding this comment

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

This is not for resultSet log handling. Because our JDBC driver uses dynamic proxies and abstract base classes, standard Java logger automatic class-detection gets confused (it would print proxy classes or abstract frameworks as the log source) sometimes. I overloaded these methods to avoid that confusion.

@Neenu1995 Neenu1995 requested a review from logachev May 5, 2026 18:28
}

protected SQLException logAndCreateException(SQLException ex) {
if (this.statement != null) {
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.

nit: usually it is better to avoid nesting, for multiple conditions easier to read

if (this.statement == null) {
  return ex;
}
...

Comment on lines +88 to +100
if (result instanceof java.sql.CallableStatement) {
return wrap(result, java.sql.CallableStatement.class, connectionId);
} else if (result instanceof java.sql.PreparedStatement) {
return wrap(result, java.sql.PreparedStatement.class, connectionId);
} else if (result instanceof java.sql.Statement) {
return wrap(result, java.sql.Statement.class, connectionId);
} else if (result instanceof java.sql.DatabaseMetaData) {
return wrap(result, java.sql.DatabaseMetaData.class, connectionId);
} else if (result instanceof java.sql.ParameterMetaData) {
return wrap(result, java.sql.ParameterMetaData.class, connectionId);
} else if (result instanceof java.sql.ResultSetMetaData) {
return wrap(result, java.sql.ResultSetMetaData.class, connectionId);
}
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.

Can you add a comment about it?

if (target == null) {
return null;
}
String connectionId = extractConnectionId(target);
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.

I meant that specifically for Connection type, it can be extracted.. e.g. for everything else it 100% makes sense to propagate value from the proxy

  static <T> T wrap(Connection connection) {
    return wrap(connectio, Connection.class, connection.getConnectionId());

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