birschick-bq commented on code in PR #2729: URL: https://github.com/apache/arrow-adbc/pull/2729#discussion_r2082336438
########## go/adbc/driver/internal/driverbase/rotating_file_writer.go: ########## Review Comment: @lidavidm / @davidhcoe ### 1. Migration from SLOG 1. Logging levels don't really apply to traces. Traces have attributes, statuses or other properties which could be filtered before export. But that would be complicated to configure. That is a good candidate for using OTel Collector. 2. Currently, the `slog` output (in `go`) is set to [output to `stderr`](https://github.com/apache/arrow-adbc/blob/825d1d1c0d06ba0090c5271ef793426b6184a9ab/go/adbc/pkg/_tmpl/driver.go.tmpl#L187). Which, as @davidhcoe notes, is difficult to redirect to a file in his scenario where new processes are being spawned in the background. 3. Maybe we could transition such that if ADBC_DRIVER_{driver-name}_LOG_LEVEL is set to one of [`debug`, `info`, `warn`, `warning`, `error`], that would enable OTel traces to be exported to `stdout` (or `stderr`). That is, be the equivalent of `OTEL_TRACES_EXPORTER=console` ### 2. Location of trace files in file system 1. I would just suggest that the location be portably available to the executing user. 2. It ensures the user has the permission to write the files. 3. It reduces a possible need for configuration option if the default is reasonable. 4. It is more likely that it doesn't inadvertently expose the files to other users in the system. 5. Your suggestion of [`os.UserConfigDir()`](https://pkg.go.dev/os#UserConfigDir) is a good choice. ### 3. Choice of `adbcfile` for OTEL_TRACES_EXPORTER 1. This seems to be the the most obvious choice given the existing values for this configuration. It avoids the possible name clash. 2. From my exposure to the OTel SDK `console` exporters in different languages (`go` and `C#`) - they have different schema format. That is the main dilemma of why an existing `file` exporter has not be defined and published, yet. From my opinion, I don't think the schema matters for this use case - debug. If it was going to be consumed by some automation (i.e., OTel Collector), then schema would be important. 3. They (OTel) seem to be stuck on the schema format. But I think also they have the requirement that the contents could be used for an automated pipeline. So schema is important in this scenario. -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
