lidavidm commented on code in PR #4659:
URL: https://github.com/apache/arrow-adbc/pull/4659#discussion_r3809401618
##########
go/adbc/driver/flightsql/flightsql_adbc_test.go:
##########
@@ -368,7 +368,7 @@ func TestFlightSQLTracingProducesTraceFiles(t *testing.T) {
}
output := traceOutput.String()
- require.Contains(t, output, "FlightSQLDatabase.Open")
+ require.Contains(t, output, "FlightSQL.Database.Open")
require.Contains(t, output, "FlightSQLStatement.ExecuteQuery")
Review Comment:
Should we change the next span to "FlightSQL.Statement.ExecuteQuery" (etc)?
##########
go/adbc/driver/flightsql/flightsql_connection.go:
##########
@@ -246,7 +247,7 @@ func doGetWithResponseMetadata(ctx context.Context, client
*flightsql.Client, ti
func doGetWithTracer(ctx context.Context, cl *flightsql.Client, endpoint
*flight.FlightEndpoint, clientCache gcache.Cache, tracing adbc.OTelTracing,
opts ...grpc.CallOption) (rdr *flight.Reader, err error) {
const spanName = "FlightSQL.Connection.DoGet"
- startTime := time.Now()
Review Comment:
(why was this changed?)
##########
go/adbc/driver/flightsql/flightsql_tracing.go:
##########
Review Comment:
It seems this tries to refactor things from logging.go but duplicates it
instead?
##########
go/adbc/driver/flightsql/flightsql_database.go:
##########
@@ -369,35 +371,52 @@ func (d *databaseImpl) SetOptionDouble(key string, value
float64) error {
return d.DatabaseImplBase.SetOptionDouble(key, value)
}
-func (d *databaseImpl) Close() error {
- if d.Logger != nil {
- d.Logger.Info("FlightSQL database closed",
- "target", d.uri.String(),
- )
- }
- return d.DatabaseImplBase.Close()
+func (d *databaseImpl) Close() (err error) {
+ const spanName = "FlightSQL.Database.Close"
+ startTime := time.Now()
+ var span trace.Span
+ _, span = internal.StartSpan(context.Background(), spanName, d)
Review Comment:
Maybe a refactor for later, but I wonder if StartSpan shouldn't also return
a NewEndSpanHelper with the start time pre-configured
##########
go/adbc/driver/flightsql/flightsql_database.go:
##########
@@ -369,35 +371,52 @@ func (d *databaseImpl) SetOptionDouble(key string, value
float64) error {
return d.DatabaseImplBase.SetOptionDouble(key, value)
}
-func (d *databaseImpl) Close() error {
- if d.Logger != nil {
- d.Logger.Info("FlightSQL database closed",
- "target", d.uri.String(),
- )
- }
- return d.DatabaseImplBase.Close()
+func (d *databaseImpl) Close() (err error) {
+ const spanName = "FlightSQL.Database.Close"
+ startTime := time.Now()
+ var span trace.Span
+ _, span = internal.StartSpan(context.Background(), spanName, d)
+
+ span.AddEvent("closing",
trace.WithAttributes(attribute.String("target", d.uri.String())))
+ return closeTracing(context.Background(), &d.DatabaseImplBase,
func(flushErr error) {
+ internal.NewEndSpanHelper(span).
+ WithError(flushErr).
+ WithStartTime(startTime).
+ EndSpan()
+ })
}
-func getFlightClient(ctx context.Context, loc string, d *databaseImpl,
authMiddle *bearerAuthMiddleware, cookies flight.CookieMiddleware)
(*flightsql.Client, error) {
+type tracingLifecycle interface {
+ ForceFlushTracing(context.Context) error
+ Close() error
+}
+
+func closeTracing(ctx context.Context, lifecycle tracingLifecycle, finishSpan
func(error)) error {
+ flushErr := lifecycle.ForceFlushTracing(ctx)
+ finishSpan(flushErr)
+ shutdownErr := lifecycle.Close()
+ return errors.Join(flushErr, shutdownErr)
+}
+
+func getFlightClient(ctx context.Context, loc string, d *databaseImpl,
authMiddle *bearerAuthMiddleware, cookies flight.CookieMiddleware, span
trace.Span) (client *flightsql.Client, err error) {
middleware := []flight.ClientMiddleware{
- {
- Unary: makeUnaryLoggingInterceptor(d.Logger),
- Stream: makeStreamLoggingInterceptor(d.Logger),
- },
flight.CreateClientMiddleware(authMiddle),
{
Unary: unaryTimeoutInterceptor,
Stream: streamTimeoutInterceptor,
},
+ {Stream: responseMetadataStreamInterceptor},
Review Comment:
(We don't need to trace unary calls?)
##########
go/adbc/driver/flightsql/flightsql_database.go:
##########
@@ -369,35 +371,52 @@ func (d *databaseImpl) SetOptionDouble(key string, value
float64) error {
return d.DatabaseImplBase.SetOptionDouble(key, value)
}
-func (d *databaseImpl) Close() error {
- if d.Logger != nil {
- d.Logger.Info("FlightSQL database closed",
- "target", d.uri.String(),
- )
- }
- return d.DatabaseImplBase.Close()
+func (d *databaseImpl) Close() (err error) {
+ const spanName = "FlightSQL.Database.Close"
+ startTime := time.Now()
+ var span trace.Span
+ _, span = internal.StartSpan(context.Background(), spanName, d)
+
+ span.AddEvent("closing",
trace.WithAttributes(attribute.String("target", d.uri.String())))
+ return closeTracing(context.Background(), &d.DatabaseImplBase,
func(flushErr error) {
+ internal.NewEndSpanHelper(span).
+ WithError(flushErr).
+ WithStartTime(startTime).
+ EndSpan()
+ })
}
-func getFlightClient(ctx context.Context, loc string, d *databaseImpl,
authMiddle *bearerAuthMiddleware, cookies flight.CookieMiddleware)
(*flightsql.Client, error) {
+type tracingLifecycle interface {
+ ForceFlushTracing(context.Context) error
+ Close() error
+}
+
+func closeTracing(ctx context.Context, lifecycle tracingLifecycle, finishSpan
func(error)) error {
+ flushErr := lifecycle.ForceFlushTracing(ctx)
+ finishSpan(flushErr)
+ shutdownErr := lifecycle.Close()
+ return errors.Join(flushErr, shutdownErr)
+}
+
+func getFlightClient(ctx context.Context, loc string, d *databaseImpl,
authMiddle *bearerAuthMiddleware, cookies flight.CookieMiddleware, span
trace.Span) (client *flightsql.Client, err error) {
middleware := []flight.ClientMiddleware{
- {
- Unary: makeUnaryLoggingInterceptor(d.Logger),
- Stream: makeStreamLoggingInterceptor(d.Logger),
- },
flight.CreateClientMiddleware(authMiddle),
{
Unary: unaryTimeoutInterceptor,
Stream: streamTimeoutInterceptor,
},
+ {Stream: responseMetadataStreamInterceptor},
}
if d.enableCookies {
middleware = append(middleware,
flight.CreateClientMiddleware(cookies))
}
- uri, err := url.Parse(loc)
+ var uri *url.URL
+ uri, err = url.Parse(loc)
if err != nil {
- return nil, adbc.Error{Msg: fmt.Sprintf("Invalid URI '%s': %s",
loc, err), Code: adbc.StatusInvalidArgument}
+ err = adbc.Error{Msg: fmt.Sprintf("Invalid URI '%s': %s", loc,
err), Code: adbc.StatusInvalidArgument}
+ return nil, err
Review Comment:
nit: why are there these changes that seemingly have no effect and just
reword code?
##########
go/adbc/driver/flightsql/flightsql_connection.go:
##########
@@ -1344,65 +1345,79 @@ func (c *connectionImpl) prepareSubstrait(ctx
context.Context, plan flightsql.Su
}
// Close closes this connection and releases any associated resources.
-func (c *connectionImpl) Close() error {
+func (c *connectionImpl) Close() (err error) {
+ const spanName = "FlightSQL.Connection.Close"
+ startTime := time.Now()
+ ctx, span := internal.StartSpan(context.Background(), spanName, c)
+ defer func() {
+ internal.NewEndSpanHelper(span).
+ WithError(err).
+ WithStartTime(startTime).
+ EndSpan()
+ }()
+
if c.cl == nil {
- return adbc.Error{
+ err = adbc.Error{
Msg: "[Flight SQL Connection] trying to close already
closed connection",
Code: adbc.StatusInvalidState,
}
+ return err
}
Review Comment:
Same here...is the intent to make sure the deferred function sees the
updated `err`? Because AFAIK, this isn't necessary.
https://go.dev/play/p/IuU7_RrPB_9
##########
go/adbc/driver/flightsql/flightsql_database.go:
##########
@@ -369,35 +371,52 @@ func (d *databaseImpl) SetOptionDouble(key string, value
float64) error {
return d.DatabaseImplBase.SetOptionDouble(key, value)
}
-func (d *databaseImpl) Close() error {
- if d.Logger != nil {
- d.Logger.Info("FlightSQL database closed",
- "target", d.uri.String(),
- )
- }
- return d.DatabaseImplBase.Close()
+func (d *databaseImpl) Close() (err error) {
+ const spanName = "FlightSQL.Database.Close"
+ startTime := time.Now()
+ var span trace.Span
+ _, span = internal.StartSpan(context.Background(), spanName, d)
+
+ span.AddEvent("closing",
trace.WithAttributes(attribute.String("target", d.uri.String())))
+ return closeTracing(context.Background(), &d.DatabaseImplBase,
func(flushErr error) {
+ internal.NewEndSpanHelper(span).
+ WithError(flushErr).
+ WithStartTime(startTime).
+ EndSpan()
+ })
}
-func getFlightClient(ctx context.Context, loc string, d *databaseImpl,
authMiddle *bearerAuthMiddleware, cookies flight.CookieMiddleware)
(*flightsql.Client, error) {
+type tracingLifecycle interface {
+ ForceFlushTracing(context.Context) error
+ Close() error
+}
+
+func closeTracing(ctx context.Context, lifecycle tracingLifecycle, finishSpan
func(error)) error {
Review Comment:
(This seems to only be used from one place? Why not just inline it?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
