feat(go/adbc): refactor logging instrumentation into OTel tracing - part 2/3 - #4659
Conversation
… dev/birschick-bq/flight-sql-log-to-trace-4-connection
… dev/birschick-bq/flight-sql-log-to-trace-4-connection
| const spanName = "FlightSQL.Database.Close" | ||
| startTime := time.Now() | ||
| var span trace.Span | ||
| _, span = internal.StartSpan(context.Background(), spanName, d) |
There was a problem hiding this comment.
Maybe a refactor for later, but I wonder if StartSpan shouldn't also return a NewEndSpanHelper with the start time pre-configured
| Unary: unaryTimeoutInterceptor, | ||
| Stream: streamTimeoutInterceptor, | ||
| }, | ||
| {Stream: responseMetadataStreamInterceptor}, |
There was a problem hiding this comment.
(We don't need to trace unary calls?)
There was a problem hiding this comment.
It seems this tries to refactor things from logging.go but duplicates it instead?
There was a problem hiding this comment.
It is duplicated only because the return types differ.
There was a problem hiding this comment.
Ah, right. It does bother me but it's unavoidable then.
…ight-sql-log-to-trace-4-connection
lidavidm
left a comment
There was a problem hiding this comment.
LGTM, though there's still many instances of unnecessary assignment before return that could be reverted
I'll fix these and then give you a heads-up. Thanks. |
… - part 3/3 (#4661) This pull request makes significant improvements to the FlightSQL driver's tracing and logging, replacing legacy logging with structured OpenTelemetry tracing, enhancing error recording, and ensuring better test coverage for tracing-related cleanup. The changes modernize how operation metadata is captured and reported, making tracing more consistent and useful for observability and debugging. **Tracing and Logging Modernization:** * Replaced `slog`-based logging with OpenTelemetry tracing throughout `flightsql_statement.go` and `flightsql_bulk_ingest.go`, using `attribute.KeyValue` for structured event attributes and adding detailed span events for operation start and finish. This includes updating helper functions to return attributes in the new format. * Updated error handling in tracing: errors are now consistently recorded as span errors with stack traces, and operation-specific error wrapping is improved for clarity. **API and Internal Refactoring:** * Refactored method signatures and internal calls to use the new tracing and attribute formats, including changes to `SetSqlQuery`, `ExecuteQuery`, `ExecuteUpdate`, and `Prepare` methods in `statement`. * Changed the construction of the Flight client to add a unary interceptor for response metadata, improving consistency for both unary and stream calls. **Testing Improvements:** * Added a new test `TestFlightSQLTracingCleansUpAfterConstructionFailure` to ensure that tracing resources are properly cleaned up even when database construction fails, preventing resource leaks. **Dependency and Import Updates:** * Updated imports to include OpenTelemetry packages and internal helpers for tracing, and removed unused logging imports. **Helper and Utility Updates:** * Updated helper functions for generating tracing attributes and correlation metadata to use the new attribute-based approach, improving code clarity and maintainability. These changes collectively ensure that the driver emits rich, structured telemetry for all major operations, facilitates easier debugging, and aligns with modern observability practices. --- Extends: #4659 Part 3/3 --------- Co-authored-by: Bruce Irschick (Bit Quill Technologies Inc) <v-birschick@microsoft.com>
This pull request refactors the FlightSQL ADBC driver to improve error handling, tracing, and code clarity. The main focus is on replacing logger-based tracing with OpenTelemetry (OTel) tracing, enhancing context propagation, and ensuring errors are consistently handled and returned. Several methods now include OTel tracing spans, and error handling is standardized across the connection implementation.
Tracing and Observability Improvements
doGetWithLogger) with OTel-based tracing (doGetWithTracer) throughout the connection implementation, ensuring all relevant methods now emit OTel spans for better observability. ([[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL384-L472),[[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL855-R783),[[3]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1112-R1132))PrepareDriverInfo,GetObjectsCatalogs,GetObjectsDbSchemas,GetObjectsTables, andGetTableSchema, capturing start time, errors, and context for each operation. ([[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL821-R743),[[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL937-R873),[[3]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL971-R948),[[4]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1016-R984),[[5]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1096-R1081))Error Handling and API Consistency
[[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL892-R832),[[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL955-R899),[[3]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL971-R948),[[4]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1016-R984),[[5]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1047-R1016),[[6]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1077-R1045),[[7]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1112-R1132),[[8]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1162-R1159))[[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL821-R743),[[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL937-R873),[[3]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL971-R948),[[4]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1016-R984),[[5]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1096-R1081),[[6]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1162-R1159))Code Cleanup and Minor Fixes
log/slog) and the obsoletedoGetWithLoggerfunction. ([[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL26),[[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL384-L472))[[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL249-R250),[[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL842-R765),[[3]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL855-R783),[[4]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL955-R899),[[5]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1047-R1016),[[6]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1077-R1045),[[7]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1112-R1132),[[8]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1162-R1159))Test and Output Updates
[go/adbc/driver/flightsql/flightsql_adbc_test.goL371-R371](https://github.com/apache/arrow-adbc/pull/4659/files#diff-1147cdf8ab7a5dcb1d84bcb2b7a2185cf2fc3802b7eb3718bffadfe990ecdf56L371-R371))Documentation and Comments
[go/adbc/driver/flightsql/flightsql_connection.goR236-R237](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cR236-R237))These changes collectively improve the maintainability, observability, and robustness of the FlightSQL ADBC driver.Refactors logging instrumentation into OTel tracing
otelgrpctracingtracingLifecycle)Extends: #4655
Is part 2 of 3 changes.