Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go/adbc/driver/flightsql: add extra logging #492

Closed
5 tasks
lidavidm opened this issue Mar 6, 2023 · 5 comments · Fixed by #1048
Closed
5 tasks

go/adbc/driver/flightsql: add extra logging #492

lidavidm opened this issue Mar 6, 2023 · 5 comments · Fixed by #1048
Assignees

Comments

@lidavidm
Copy link
Member

lidavidm commented Mar 6, 2023

grpc-go doesn't log outgoing headers - ideally we should have a way to enable logging of headers + Flight/Flight SQL operation for debugging

  • Incoming, outgoing headers (optionally, since this may leak credentials)
  • Incoming, outgoing headers (header names only)
  • Requests made to each server, and server locations (and semantically, 'which connection' the requests correspond to)
  • Bake in some understanding of OpenTelemetry?
  • The raw gRPC error
@rtadepalli
Copy link
Contributor

Task 1 seemed straightforward so I tried to log the headers by registering an interceptor (similar to unaryTimeoutInterceptor) while creating the flight client and then it occurred to me -- should this logging happen in the arrow flight client library instead of within flightsql_adbc.go? May be easier to get the headers there, but I might be missing something.

Also, there's no logging package present in go.mod, so I am not sure if there is a preference for which package to add or if we should just write to stdout. Thanks.

@lidavidm
Copy link
Member Author

lidavidm commented Jun 9, 2023

Possibly. @zeroshade opinions?

That said it would be an interceptor either way - we can always refactor or upstream things and I think we can iterate more quickly here.

I'm not sure about the state of Go logging. Ideally a structured logging package, compatible with OpenTelemetry (but configurable via environment variable for easy debugging), would be used but I'm not sure if that's available.

@zeroshade
Copy link
Member

So the base Arrow client flight library has a function that can be used to easily construct interceptors from simple interfaces to grab the headers. It's pretty easy to create a middleware to perform logging using these interfaces.

As far as logging in Go, for most cases the standard logging package is more than good enough. But if you want more structured logging zap, zerolog, and apex are the most popular structured logging libraries currently.

For something like ADBC, I'd say we're probably best off setting up a logging interface to allow users to plug whatever logger they want (i.e. follow the stdlib logger's interface because most logging libraries support that interface for compatibility).

For OpenTelemetry, we definitely need to add otel instrumentation to the base arrow flight and flightsql packages, it's not hard. The package is very good and we already pass the context around which would contain the necessary headers etc. It's on my list of things to do, but I'd highly welcome and review any PRs for it!

@lidavidm
Copy link
Member Author

Punting on this for the next release, but @zeroshade are you suggesting we essentially add to the ADBC interface so you can set a logger on the Database object (or similar)? (And then we can figure out how to bind that through to say C/C++ and so on; possibly we want a SetOptionFuncPointer or something?)

@lidavidm lidavidm self-assigned this Aug 28, 2023
@lidavidm
Copy link
Member Author

It looks like slog has a backport, so we could define the extension interface, log to a driver-wide logger, and have the FFI part configure the logger (using some standard environment variables) for debugging.

lidavidm added a commit to lidavidm/arrow-adbc that referenced this issue Sep 11, 2023
lidavidm added a commit to lidavidm/arrow-adbc that referenced this issue Sep 11, 2023
lidavidm added a commit to lidavidm/arrow-adbc that referenced this issue Sep 11, 2023
lidavidm added a commit to lidavidm/arrow-adbc that referenced this issue Sep 12, 2023
lidavidm added a commit that referenced this issue Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants