feat(jdbc): basic OpenTelemetry tracing integration for BigQuery JDBC Statement#12124
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes the foundational OpenTelemetry tracing capabilities within the BigQuery JDBC driver. It allows for detailed observability of database interactions by automatically creating spans for JDBC query, update, and batch operations, enriching them with query details, execution times, and error information. This integration is designed to be non-disruptive, providing valuable insights into driver performance and behavior without altering existing workflows. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces foundational OpenTelemetry tracing for the BigQuery JDBC driver's statement execution, which is a valuable addition for observability. The implementation correctly handles context propagation for asynchronous operations like pagination and streaming, and follows standard OpenTelemetry patterns for creating spans and recording exceptions. I have provided a couple of suggestions: one is a minor correction for a copyright year, and the other is a recommendation to unify the exception handling logic within the new tracing code for improved consistency and maintainability. Overall, this is a solid contribution.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This PR introduces OpenTelemetry tracing for JDBC statements, which is a great feature for observability. The implementation correctly wraps statement execution methods to create spans, and it handles context propagation to asynchronous tasks. My review includes two main suggestions for improvement: refactoring duplicated tracing logic in BigQueryStatement to improve maintainability, and enhancing exception logging in BigQueryJdbcOpenTelemetry to retain stack traces for better diagnostics. Overall, the changes are well-structured and provide a solid foundation for tracing.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates OpenTelemetry into the BigQuery JDBC driver to enable tracing for query execution, batch operations, and pagination. It introduces a new connection property, 'enableDefaultTelemetryExporter', and allows users to provide a custom OpenTelemetry instance. A critical compilation error was identified in the 'withTracing' method within BigQueryStatement.java, where a broad Exception is caught and rethrown, violating the method's checked exception signature.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry tracing to the BigQuery JDBC driver by adding necessary dependencies, a telemetry utility class, and wrapping statement execution and pagination with tracing spans. It also enables telemetry configuration through connection properties and the DataSource class. A critical bug was noted in BigQueryDriver where passing a customOpenTelemetry object via connection properties results in it being incorrectly stringified into the URL, breaking programmatic injection; the reviewer recommends using the DataSource constructor to preserve the object reference.
|
/gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry tracing to the BigQuery JDBC driver. Key changes include adding OpenTelemetry dependencies to the project, introducing a new connection property enableDefaultTelemetryExporter, and allowing a customOpenTelemetry instance to be passed through connection properties. The implementation wraps core execution methods in BigQueryStatement with tracing logic and ensures that asynchronous background tasks, such as result pagination and stream processing, correctly propagate the telemetry context. I have no feedback to provide as there were no review comments to evaluate.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates OpenTelemetry into the BigQuery JDBC driver to provide tracing for query execution and asynchronous tasks. It introduces new connection properties for enabling GCP trace and log exporters, allows for a custom OpenTelemetry instance, and wraps key statement execution methods and background threads with tracing spans and context. Feedback was provided to add a private constructor to the BigQueryJdbcOpenTelemetry utility class to follow best practices for utility classes.
| sb.append(query); | ||
| } | ||
| String combinedQueries = sb.toString(); | ||
| span.setAttribute("db.statement", combinedQueries); |
There was a problem hiding this comment.
Should we define a separate attribute for a list of strings instead?
There was a problem hiding this comment.
Yes, and I also realised I can store the batch queries as a structured string array attribute (db.batch.statements) instead of a concatenated string.
| } | ||
| Context.current() | ||
| .wrap( | ||
| () -> { |
There was a problem hiding this comment.
At this point, should we move lambdas to functions instead?
There was a problem hiding this comment.
Yeah, it is getting quite messy.
There was a problem hiding this comment.
I moved these two to new functions
| @Override | ||
| public boolean execute(String sql) throws SQLException { | ||
| LOG.finest("++enter++"); | ||
| Tracer tracer = getSafeTracer(); |
There was a problem hiding this comment.
Can probably add a generic method private T traceMethod(String spanName, Runnable<T> ) and pass Impl method as a lambda
There was a problem hiding this comment.
Hm I am a bit confused because the previous version of PR had that exact pattern (using a withTracing helper that took a lambda), but I refactored it because I interpreted your previous comment:
I'd suggest using executeQueryImpl method for core logic instead of moving it to lambda. So that way public interface method will be tracer-wrapper on top of actual executeQueryImpl
as a preference for avoiding lambdas in these execution methods.
There was a problem hiding this comment.
Can probably add a generic method private T traceMethod(String spanName, Runnable ) and pass Impl method as a lambda
Done
| this.enableGcpTraceExporter, this.enableGcpLogExporter, this.customOpenTelemetry); | ||
| if (this.enableGcpTraceExporter || this.customOpenTelemetry != null) { | ||
| this.tracer = BigQueryJdbcOpenTelemetry.getTracer(openTelemetry); | ||
| bigQueryOptions.setOpenTelemetryTracer(this.tracer); |
There was a problem hiding this comment.
What about the other clients? Do they support OTel?
There was a problem hiding this comment.
Yes, the BigQueryReadSettings also supports it. I have added it.
There was a problem hiding this comment.
There is a write client as well.
There was a problem hiding this comment.
Yeah, I checked the source for BigQueryWriteSettings in the parent project, and unlike the read client settings, it does not expose a setOpenTelemetryTracerProvider(...) method.
Also, I think its not needed, its network calls should be traced as children of our active spans via context propagation, once it is fully implemented
Neenu1995
left a comment
There was a problem hiding this comment.
LGTM. But add the write client as well if it supports OTel.
b/491239772
b/491239773
Changes
New connection properties:
enableGcpTraceExporter(Boolean, default: false) andenableGcpLogExporter(Boolean, default: false)customOpenTelemetry(Instance): Programmatic injection of a custom SDK (User Application-Managed setup) viaBigQueryDataSource.setCustomOpenTelemetry()Added the core initialization logic for
OpenTelemetry. During connection setup, it evaluates whether tracing is enabled and constructs an OpenTelemetry Tracer instance. Then, it passes this newly minted tracer strictly downward into the coreBigQueryOptions.Buildervia.setOpenTelemetryTracer()Intercepted the execution functions (
execute,executeQuery,executeLargeUpdate,executeBatch) to spawn child spans wrapping each database call.