feat(go/adbc): refactor logging instrumentation into OTel tracing - part 1/3 - #4655
Conversation
birschick-bq
changed the title
|
Is there some more meaningful way to split this up than part 1/2/3? Can we break it into parts that can each have meaningful descriptions? It's not clear to me how these changes were grouped |
Yes. I'll work on it. |
|
Ok, thanks. Let me know if that's too much of a hassle. I wish the stacked PRs feature worked for us...I gave it a try, but it means you/others need push access to this repo which is a non-starter (even for committers I'd rather we not push dev branches to this repo). Hopefully that gets fixed. |
Without a re-write here are the individual stacked PRs for part 2 and 3 on my repo @lidavidm - Let me know if this is satisfactory. Thanks. |
|
@davidhcoe - if you have time to review, this refactors your logging instrumentation into OTel tracing. |
| // EndSpanWithRecordedError ends a span whose error event has already been | ||
| // recorded by the caller, setting only the final status and error type. | ||
| func EndSpanWithRecordedError(span trace.Span, err *error, options ...trace.SpanEndOption) { | ||
| if err != nil { |
There was a problem hiding this comment.
The above function also checks for *err != nil; while it makes less sense to pass &err into this function, perhaps they should be consistent?
There was a problem hiding this comment.
There's gotta be a better way than making every permutation of helper function. A builder/fluent-style API perhaps?
| } | ||
| collector.mutex.Lock() | ||
| collector.value = value.Copy() | ||
| collector.mutex.Unlock() |
| for _, loc := range endpoint.Location { | ||
| uris = append(uris, loc.Uri) | ||
| } | ||
| attrs = append(attrs, attribute.StringSlice("locations", uris)) |
There was a problem hiding this comment.
(1) Is this actually useful to record?
(2) While it doesn't apply to gRPC, some implementations may be putting things like presigned URLs into "locations" (I've seen this before)
3 participants
This pull request introduces OpenTelemetry tracing to the FlightSQL driver, enhancing observability for record reading and endpoint streaming operations. The main changes involve adding tracing hooks, attributes, and error recording to critical code paths, as well as refactoring logging and context management to support tracing. Additionally, new utility functions for collecting and attaching response metadata to traces are implemented.
Tracing and Observability Enhancements:
record_reader.go, including span creation, event recording, and error tracking innewRecordReaderand endpoint goroutines. This enables detailed tracing of FlightSQL record reading operations. [1] [2] [3] [4]flightsql_tracing.gofor collecting response metadata, building trace attributes for endpoints, and summarizing stream progress as OpenTelemetry attributes.Refactoring for Tracing Integration:
logging.gowith tracing attribute builders, and removed now-redundant logging functions. [1] [2]record_reader.goto usecontext.CancelCauseFuncfor improved error propagation and tracing. [1] [2]Internal API and Dependency Updates:
recordReaderConfigstruct to include a tracing configuration parameter, enabling tracing to be passed through to record readers.These changes collectively provide fine-grained tracing and error visibility for FlightSQL operations, making it easier to monitor, debug, and analyze the driver's behavior in production environments.
Refactors slog instrumentation into OTel tracing.
flightsql_tracing.goto provide tracing wrappers originally provided inlogging.godriverbaseto handle improved trace handlingPart 1 of a multi-part change to refactor logging instrumentation into OTel tracing.
Stacked PRs in my personal forked repo ...
Part 2
Part 3