| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
This pull request implements OpenTelemetry SDK initialization and OTLP exporting for the BigQuery JDBC driver. It introduces new connection properties for telemetry credentials and project IDs, and implements a caching mechanism for OTel SDK instances. Review feedback identifies a potential bug in the caching logic where configuration flags are missing from the cache key, and a resource leak caused by registering multiple shutdown hooks. Suggestions were provided to include flags in the cache key and use a single global shutdown hook to manage SDK lifecycle.
Sorry, something went wrong.
|
/gemini review |
Sorry, something went wrong.
There was a problem hiding this comment.
This pull request implements auto-configuration and caching for OpenTelemetry SDK instances within the BigQuery JDBC driver. It introduces new connection properties for GCP telemetry credentials and project IDs, enabling more flexible telemetry setup. The review feedback identifies critical risks regarding potential NullPointerExceptions when unboxing Boolean flags and suggests a more robust cache key construction to prevent key collisions in the SDK cache.
Sorry, something went wrong.
|
/gemini review |
Sorry, something went wrong.
There was a problem hiding this comment.
This pull request implements OpenTelemetry auto-configuration for the BigQuery JDBC driver, enabling the use of GCP-specific telemetry credentials and project IDs. It introduces a caching layer for OpenTelemetrySdk instances and a shutdown hook for resource cleanup. Feedback points out that the OTLP exporter configuration lacks proper authentication for GCP and identifies a potential memory leak in connection management. Further suggestions include refining the SDK caching logic to remove redundant keys and ensuring the shutdown hook is exception-safe.
Sorry, something went wrong.
| private final String credentialsHashOrPath; | ||
| private final boolean enableTrace; | ||
|
|
||
| SdkCacheKey(String projectId, String credentialsHashOrPath, boolean enableTrace) { |
There was a problem hiding this comment.
QQ: Why couldn't the projectId alone suffice as the key?
Is there a use case where same projectId would have different credentials or different values for enableTrace ?
Sorry, something went wrong.
There was a problem hiding this comment.
We need all three fields to support multiple concurrent connections in a shared JVM:
Sorry, something went wrong.
There was a problem hiding this comment.
I see, makes sense. Thanks for explaining.
Sorry, something went wrong.
| // closed. Logging is avoided here because the logging system might have | ||
| // already been shut down by the JVM. |
There was a problem hiding this comment.
The shutdown sequesnce seems to imply that the shutdown hooks are executed in the beginning of the shutdown sequence.
So IIUC, this means the logger would still be active. Did you come across any errors when trying to use a logger here to log error messages?
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah, so I did read about it and while it is true that shutdown hooks run at the start of JVM shutdown, Java's LogManager registers its own JVM shutdown hook to clean up resources and close all active logging handlers.
Since the JVM executes registered shutdown hooks in a concurrent and arbitrary order, we cannot guarantee whether our telemetry shutdown hook or the LogManager's shutdown hook will execute first. Therefore, avoiding logging inside shutdown hooks is a defensive best practice in Java to prevent race conditions and unreliable logs during VM termination.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
b/491238299
b/511147053
This PR completes the implementation of the OpenTelemetry SDK lifecycle and cross-project authentication for the BigQuery JDBC driver. It introduces thread-safe caching of heavy OTel SDK instances to support multi-project tracing without global side effects.
Changes
BigQueryJdbcOpenTelemetry.java
BigQueryJdbcUrlUtility.java
BigQueryJdbcOAuthUtility.java
BigQueryConnection.java
BigQueryJdbcOpenTelemetryTest.java
BigQueryArrowStructTest.java