feat(bqjdbc): Implement Correlated OpenTelemetry Logging Bridge#13002
feat(bqjdbc): Implement Correlated OpenTelemetry Logging Bridge#13002keshavdandeva wants to merge 10 commits intojdbc/feature-branch-otelfrom
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 introduces OpenTelemetry and Google Cloud Logging support for the BigQuery JDBC driver, including a new OpenTelemetryJulHandler to bridge logs and logic to suppress local logging in cloud-only mode. The review identified several critical issues regarding resource management and logging correctness: a significant handler leak occurs because new handlers are attached to a shared logger instance for every connection without being removed, the handler incorrectly logs raw message templates instead of formatted strings, and the root logger fails to clean up existing file handlers when switching to cloud-only logging.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a logging bridge from java.util.logging to OpenTelemetry and Google Cloud Logging, including a new OpenTelemetryJulHandler and connection-specific telemetry management. Key feedback focuses on making the handler registration idempotent to avoid duplicates, ensuring global logs are not silently dropped when connection IDs are absent, and correctly formatting trace IDs for GCP correlation. Further improvements were suggested regarding exception safety during client flushing and clarifying parameter naming in the telemetry configuration logic.
| BigQueryJdbcOpenTelemetry.getOpenTelemetry( | ||
| this.enableGcpTraceExporter, this.enableGcpLogExporter, this.customOpenTelemetry); | ||
|
|
||
| if (this.enableGcpLogExporter || this.customOpenTelemetry != null) { |
There was a problem hiding this comment.
Feels weird to check this.customOpenTelemetry, but use openTelemetry. Is it intended?
| if (logPath == null) { | ||
| logPath = BigQueryJdbcUrlUtility.DEFAULT_LOG_PATH; | ||
| // Cloud-Only Mode: Suppress local file creation if GCP log exporter is enabled | ||
| if (ds.getEnableGcpLogExporter() && logLevel != Level.OFF) { |
There was a problem hiding this comment.
What's special about Level.OFF? If it is off, does it matter what logPath value is?
Also based on the comment, if intent is to suppress file logging, it should be done outside of if (logPath == null) check
| logger.removeHandler(h); | ||
| } | ||
| fileHandler = null; | ||
| logger.setLevel(Level.OFF); |
There was a problem hiding this comment.
Better to move logger.setLevel(level) outside of the if, it happens in both code paths
| public class BigQueryJdbcOpenTelemetry { | ||
|
|
||
| static final String INSTRUMENTATION_SCOPE_NAME = "com.google.cloud.bigquery.jdbc"; | ||
| static final String BIGQUERY_NAMESPACE = "com.google.cloud.bigquery"; |
There was a problem hiding this comment.
nit: Should it be jdbc namespace?
| try { | ||
| // Extract connection ID from baggage | ||
| String connectionId = | ||
| Baggage.fromContext(Context.current()).getEntryValue("jdbc.connection_id"); |
There was a problem hiding this comment.
nit: move magic string to some constant
| connectionId = BigQueryJdbcMdc.getConnectionId(); | ||
| } | ||
|
|
||
| if (connectionId == null) { |
There was a problem hiding this comment.
Should we proceed with null connectionId` instead?
| String spanId = spanContext.isValid() ? spanContext.getSpanId() : null; | ||
|
|
||
| // TODO(b/491238299): May require refinement for structured logging or error handling | ||
| if (loggingClient == null) { |
There was a problem hiding this comment.
nit: not needed, you already checked it is non-null before; If there is another code path with null loggingClient, it is better to catch the crash in unit/integration tests instead of silently dropping logs
| } | ||
|
|
||
| private com.google.cloud.logging.Severity mapGcpSeverity(Level level) { | ||
| if (level == Level.SEVERE) return com.google.cloud.logging.Severity.ERROR; |
There was a problem hiding this comment.
Please replace with switch/case
| } | ||
|
|
||
| private void publishToOTel(LogRecord record, String connectionId, OpenTelemetry openTelemetry) { | ||
| if (openTelemetry == null) { |
| final Logging loggingClient; | ||
| final boolean useDirectGcpLogging; | ||
|
|
||
| TelemetryConfig( |
There was a problem hiding this comment.
What's the purpose of this config? Is there difference between openTelemetry clients/loggingClients?
2 participants
b/496678357
This PR implements the Correlated GCP Logging Bridge for the OpenTelemetry integration in the BigQuery JDBC driver. It enables bridging standard Java logs (
java.util.logging) to the OpenTelemetry Logs API, allowing users to correlate logs with distributed traces and isolate them by connection session.Changes
BigQueryDriver.java: Implemented Cloud-Only Mode matrix logic to suppress local file creation whenenableGcpLogExporter=trueandLogPathis omitted.BigQueryJdbcRootLogger.java: UpdatedsetLevelto handleLevel.OFFproperly and skip file handler creation if path is null.BigQueryConnection.java: AttachedOpenTelemetryJulHandlerto the"com.google.cloud.bigquery"namespace during initialization.OpenTelemetryJulHandler.java: Created a new handler that bridges JUL logs to OTel Logs API with context harvesting and connection ID filtering.pom.xml: Addedgoogle-cloud-loggingdependency with version3.33.0-SNAPSHOTand auto-update marker.OpenTelemetryJulHandlerTest.java: Created unit tests usingOpenTelemetryExtensionto verify log emission and filtering.