test(bqjdbc): add e2e otel test and code refinements#13226
test(bqjdbc): add e2e otel test and code refinements#13226keshavdandeva wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces default limits (60KB) for OpenTelemetry attribute values to prevent ingestion failures in GCP Cloud Trace. It also exposes the connection ID, configures the JUL handler to capture all log levels, and adds comprehensive unit and integration tests for OpenTelemetry integration. Feedback focuses on improving the new integration test by switching from JUnit 4 to JUnit 5 for consistency and adhering to standard naming conventions for static constants.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry support for the BigQuery JDBC driver, including GCP exporters for traces and logs, and adds comprehensive integration tests. Key implementation details include setting default attribute length limits to prevent Cloud Trace ingestion failures and exposing the connection ID for telemetry correlation. Review feedback highlights an accidental dependency removal in the BOM, discourages hardcoding versions in the POM, suggests a more conservative attribute length limit to ensure span ingestion, and notes the importance of correctly handling InterruptedException in test utilities.
I am having trouble creating individual review comments. Click here to see my feedback.
gapic-libraries-bom/pom.xml (304-310)
The removal of google-cloud-bigtable-deps-bom appears to be accidental and is unrelated to the OpenTelemetry changes described in this PR. This could break downstream projects or automation that relies on this BOM entry.
java-bigquery/google-cloud-bigquery-jdbc/pom.xml (383)
Hardcoding the version 2.92.0 for google-cloud-trace is discouraged. If this dependency is managed by the project's BOM, the version should be omitted. If not, consider defining a property in the parent POM to manage this version consistently.
java-bigquery/google-cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcOpenTelemetry.java (72)
The default limit of 61,440 characters is very close to the 64KB byte limit of GCP Cloud Trace. Since this limit is per attribute, and a span can have multiple attributes plus other metadata, a single 60KB attribute could still cause the entire span to exceed the 64KB backend limit (especially if non-ASCII characters are present, as Java characters are UTF-16). Consider a more conservative default, such as 32768 (32KB), to ensure the span remains within safe ingestion limits.
private static final String DEFAULT_ATTRIBUTE_LENGTH_LIMIT = "32768";
java-bigquery/google-cloud-bigquery-jdbc/src/test/java/com/google/cloud/bigquery/jdbc/it/ITOpenTelemetryTest.java (255-257)
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.
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException("Test interrupted", e);
} catch (Exception e) {
// Ignore exceptions during remote lookup and retry
}References
- 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.
a8c1516 to
9780ed9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry refinements and integration tests for the BigQuery JDBC driver, including the implementation of default attribute length limits, making getConnectionId() public, and configuring the OpenTelemetryJulHandler to capture all log levels. Feedback identifies a discrepancy between the intended 60KB attribute limit and the 32KB constant used, as well as a potentially accidental deletion of a Bigtable dependency in the BOM file.
I am having trouble creating individual review comments. Click here to see my feedback.
gapic-libraries-bom/pom.xml (304-310)
The removal of the google-cloud-bigtable-deps-bom dependency appears to be unrelated to the stated purpose of this pull request (BigQuery JDBC OTel tests and refinements). Please verify if this deletion was intentional, as it could impact downstream projects relying on this BOM.
java-bigquery/google-cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcOpenTelemetry.java (72)
There is a discrepancy between the PR description (and the code comments on lines 298-300) and the actual value of DEFAULT_ATTRIBUTE_LENGTH_LIMIT. The description specifies a 60KB limit (61,440 characters), but the constant is set to 32768 (32KB).
private static final String DEFAULT_ATTRIBUTE_LENGTH_LIMIT = "61440";
References
- Ensure that comments and constants reflect the current configuration accurately to maintain logical consistency.
b/499079838
Changes
1. BigQueryJdbcOpenTelemetry.java
60KB (61,440 characters)to the autoconfigured OpenTelemetry instance properties.BigQueryExceptionstack traces or Arrow schema payloads exceeding the hard 64KB Cloud Trace backend limits.2. OpenTelemetryJulHandler.java
Level.ALLin the constructor.Level.INFOand silently dropFINE/DEBUGqueries. Delegates log filtering exclusively to the Connection loggers.3. BigQueryConnection.java
getConnectionId()from package-private topublic.4. ITOpenTelemetryTest.java
testExecute_withOpenTelemetryGcpExporter): Natively resolves target project viaServiceOptions.getDefaultProjectId(). Runs an optimized in-memory array query and iterates results to trigger small-page JSON pagination. Queries Cloud Trace E2E to strictly assert that async pagination child spans are parented perfectly under the root JDBC span.testExecute_withErrorCorrelation): Triggers database failures, capturesSQLException, harvests Trace IDs from standard logs, and verifies failed span ingestion in Cloud Trace.5. BigQueryConnectionTest.java
testConnect_withCustomOpenTelemetry_usesCustomInstance) verifying the custom OTel injection pipeline.OpenTelemetryExtensionto mock an OTel provider locally. Injecting the custom SDK via properties, it validates thatBigQueryConnectionresolves the instance and routes spans exclusively to the custom provider6. pom.xml
google-cloud-tracetest-scoped dependency to query Cloud Trace v1 API programmatically during E2E validation.