feat(bigquery-jdbc): record Statement execution and ConnectionAttempts

[Neenu1995]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces client-side usage and diagnostic telemetry to the BigQuery JDBC driver, enabling the background collection and periodic upload of anonymous metrics such as connection attempts, statement executions, errors, and feature usage. The feedback recommends aligning the implemented configuration keys with the documentation, catching Exception instead of Throwable in BigQueryDriver to avoid masking JVM errors, removing a redundant error-code extraction helper, properly restoring the thread interrupted status when handling InterruptedException, and adding a depth limit to the throwable causal chain traversal to prevent infinite loops.

I am having trouble creating individual review comments. Click here to see my feedback.

java-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/telemetry/v1/TelemetryConfiguration.java (158-174)

There is a significant discrepancy between the documented configuration options in README.MD and the actual implementation:

1. The environment variable and system property names used in the code (BIGQUERY_JDBC_TELEMETRY_ENABLED) do not match the ones documented in the README.MD (GOOGLE_BIGQUERY_JDBC_TELEMETRY_ENABLED and google.bigquery.jdbc.telemetry.enabled).
2. The documented connection properties TelemetryUploadInterval and TelemetryBatchSize, along with their corresponding environment variables (GOOGLE_BIGQUERY_JDBC_TELEMETRY_INTERVAL_MS, GOOGLE_BIGQUERY_JDBC_TELEMETRY_BATCH_SIZE) and system properties (google.bigquery.jdbc.telemetry.interval_ms, google.bigquery.jdbc.telemetry.batch_size), are completely unparsed and unimplemented in both DataSource.java and TelemetryConfiguration.java.

Please update the property resolution logic to match the documented configuration options.

java-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryDriver.java (227-240)

Catching Throwable and wrapping JVM Errors (such as OutOfMemoryError or LinkageError) in a BigQueryJdbcException is a bad practice that can mask critical JVM failures and make debugging extremely difficult. Since only SQLException and IOException are expected to be thrown from the try block, catching Exception is sufficient and much safer. Additionally, we can use the more robust TelemetryManager.extractErrorCode(e) instead of the local private helper.

    } catch (Exception e) {
      int errorCode = TelemetryManager.extractErrorCode(e);
      TelemetryManager.recordConnectionAttempt(Status.STATUS_ERROR, errorCode, authType);
      if (e instanceof SQLException) {
        throw (SQLException) e;
      } else if (e instanceof RuntimeException) {
        throw (RuntimeException) e;
      } else if (e instanceof IOException) {
        LOG.warning("Getting a warning: " + e.getMessage());
        return null;
      } else {
        throw new BigQueryJdbcException("Failed to establish BigQuery connection", e);
      }
    }

java-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryDriver.java (318-326)

This private extractErrorCode helper method is redundant and less robust than TelemetryManager.extractErrorCode(Throwable), which also traverses the causal chain and handles BigQueryException. Consider removing this method entirely and using TelemetryManager.extractErrorCode instead.

java-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryPreparedStatement.java (350-358)

When catching InterruptedException, it is a standard Java best practice to restore the interrupted status of the thread by calling Thread.currentThread().interrupt() so that higher-level call stacks are aware of the interruption and handle it appropriately.

      } catch (DescriptorValidationException | IOException | InterruptedException e) {
        if (e instanceof InterruptedException) {
          Thread.currentThread().interrupt();
        }
        writeApiExecutionBuilder
            .setStatus(com.google.cloud.bigquery.jdbc.telemetry.v1.Status.STATUS_ERROR)
            .setErrorCode(TelemetryManager.extractErrorCode(e));
        throw new BigQueryJdbcRuntimeException("Failed to execute batch with Write API", e);
      } finally {
        long durationMs = System.currentTimeMillis() - startTime;
        TelemetryManager.recordStatementExecution(writeApiExecutionBuilder, durationMs);
      }

References

1. In Java, do not swallow InterruptedException. When catching it, restore the thread's interrupted status by calling Thread.currentThread().interrupt() and handle the interruption appropriately, such as by throwing a relevant exception to signal that the operation cannot proceed.

java-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryStatement.java (683-687)

When catching InterruptedException, it is a standard Java best practice to restore the interrupted status of the thread by calling Thread.currentThread().interrupt() so that higher-level call stacks are aware of the interruption and handle it appropriately.

    } catch (InterruptedException ex) {
      Thread.currentThread().interrupt();
      this.currentExecutionBuilder
          .setStatus(com.google.cloud.bigquery.jdbc.telemetry.v1.Status.STATUS_ERROR)
          .setErrorCode(TelemetryManager.extractErrorCode(ex));
      throw new BigQueryJdbcRuntimeException("Interrupted during runQuery", ex);

References

1. In Java, do not swallow InterruptedException. When catching it, restore the thread's interrupted status by calling Thread.currentThread().interrupt() and handle the interruption appropriately, such as by throwing a relevant exception to signal that the operation cannot proceed.

java-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/telemetry/v1/TelemetryManager.java (277-294)

To prevent potential infinite loops in case of circular causal chains in the Throwable hierarchy (which can be constructed via reflection or custom exceptions), consider adding a depth limit to the traversal loop.

  public static int extractErrorCode(Throwable t) {
    int depth = 0;
    while (t != null && depth++ < 20) {
      if (t instanceof BigQueryException) {
        int code = ((BigQueryException) t).getCode();
        if (code != 0) {
          return code;
        }
      }
      if (t instanceof SQLException) {
        int code = ((SQLException) t).getErrorCode();
        if (code != 0) {
          return code;
        }
      }
      t = t.getCause();
    }
    return 1000;
  }

[logachev]

Please prefer using early returns over nesting.

http://go/tott/733

[logachev]

This method is a part of initialization for instance which is already done in a way to guarantee it runs once. I don't think we need extra sunchronized nor even tracking of shutdownHookRegistered.

You can perform this registration before final instance = localRef and if instance is not null guarantees that shutdown hook is registered.

[logachev]

Let's avoid altering the behavior as a side-effect of the change.

Can handle IOException the way it was handled before and just re-raise t as-is without introducing new exception or losing part of the data (e.g. if exception inherited from SQLException, you'll lose that parent exception part due to type casting)

[logachev]

nit: You can just capture errorCode in exception handling and set it once in finally