Skip to content

feat(bqjdbc): Implement Correlated OpenTelemetry Logging Bridge#13002

Open
keshavdandeva wants to merge 11 commits intojdbc/feature-branch-otelfrom
jdbc/otel-jul-handler
Open

feat(bqjdbc): Implement Correlated OpenTelemetry Logging Bridge#13002
keshavdandeva wants to merge 11 commits intojdbc/feature-branch-otelfrom
jdbc/otel-jul-handler

Conversation

@keshavdandeva
Copy link
Copy Markdown
Contributor

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 when enableGcpLogExporter=true and LogPath is omitted.
  • BigQueryJdbcRootLogger.java: Updated setLevel to handle Level.OFF properly and skip file handler creation if path is null.
  • BigQueryConnection.java: Attached OpenTelemetryJulHandler to 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: Added google-cloud-logging dependency with version 3.33.0-SNAPSHOT and auto-update marker.
  • OpenTelemetryJulHandlerTest.java: Created unit tests using OpenTelemetryExtension to verify log emission and filtering.

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

@keshavdandeva
Copy link
Copy Markdown
Contributor Author

/gemini review

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

@keshavdandeva keshavdandeva marked this pull request as ready for review May 6, 2026 01:49
@keshavdandeva keshavdandeva requested review from a team as code owners May 6, 2026 01:49
BigQueryJdbcOpenTelemetry.getOpenTelemetry(
this.enableGcpTraceExporter, this.enableGcpLogExporter, this.customOpenTelemetry);

if (this.enableGcpLogExporter || this.customOpenTelemetry != 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.

Feels weird to check this.customOpenTelemetry, but use openTelemetry. Is it intended?

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.

Yes, this is intended. We want to register the connection for telemetry if either GCP logging is explicitly enabled OR if the user provided a customOpenTelemetry instance (which might want to receive logs)

Since the local openTelemetry variable above is resolved to customOpenTelemetry when it is present, passing openTelemetry to registerConnection is correct.

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) {
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.

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

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.

Also based on the comment, if intent is to suppress file logging, it should be done outside of if (logPath == null) check

The intent here, as we discussed earlier, was to enable 'Cloud-Only Mode' by default when the user omits the LogPath property but enables GCP logging. If the user explicitly provides a LogPath AND enables GCP logging, we assume they want dual logging to both destinations. Moving this check outside would prevent users from logging to both file and GCP simultaneously.

What's special about Level.OFF? If it is off, does it matter what logPath value is?

Yeah, that seems redundant. Will remove it and refactor this if-else block

public class BigQueryJdbcOpenTelemetry {

static final String INSTRUMENTATION_SCOPE_NAME = "com.google.cloud.bigquery.jdbc";
static final String BIGQUERY_NAMESPACE = "com.google.cloud.bigquery";
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: Should it be jdbc namespace?

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.

I did have that earlier but then realised the existing BigQueryJdbcRootLogger usescom.google.cloud.bigquer namespace as the root logger for the driver. So, changing it here would make it inconsistent with the file handler unless we change it there too

connectionId = BigQueryJdbcMdc.getConnectionId();
}

if (connectionId == 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.

Should we proceed with null connectionId` instead?

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.

If we proceed with a null connectionId (subsequently a null config), it will still return early because we don't have any configuration registered for global logs in our map. Gemini suggested to use GlobalOpenTelemetry.get(). But I didn't wanna pollute the host application's global telemetry space with driver logs by default.
Thinking about it now, I guess we can do the default config thing for all global logs. So, it would act as a catch-all fallback in our registry for any logs not mapped to a specific connection ID. WDYT?

}

private com.google.cloud.logging.Severity mapGcpSeverity(Level level) {
if (level == Level.SEVERE) return com.google.cloud.logging.Severity.ERROR;
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.

Please replace with switch/case

Copy link
Copy Markdown
Contributor Author

@keshavdandeva keshavdandeva May 6, 2026

Choose a reason for hiding this comment

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

Switch/case doesn't work for this function as java.util.logging.Level is a class with static constants, not an enum, so we cannot use it directly in a switch statement.

Doing so would require using hardcoded integer values in the case labels

final Logging loggingClient;
final boolean useDirectGcpLogging;

TelemetryConfig(
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.

What's the purpose of this config? Is there difference between openTelemetry clients/loggingClients?

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.

The TelemetryConfig stores the telemetry settings for a specific JDBC connection. We are using a single global handler to prevent memory leaks, that handler uses the connectionId from the log context to look up this config and know how to route the logs for that specific connection.

Is there difference between openTelemetry clients/loggingClients?

  • openTelemetry is the standard OTel SDK instance
  • loggingClient is the direct Google Cloud Logging client. We use it for the "export to GCP Mode" to send logs directly to Cloud Logging, bypassing OTel, because OTel logs are not yet natively supported in GA by Google Cloud.

@keshavdandeva keshavdandeva requested a review from logachev May 6, 2026 12:58
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