[Fix] Fixed logging for underlying Go SDK

[tanmay-db]

Changes

Go SDK doesn't contain valid context for terraform logger, due to this we get a nil logger from context: https://github.com/hashicorp/terraform-plugin-log/blob/main/tflog/provider.go#L40.

We always use the context from terraform and ignore the one from Go SDK.

Tests

TF_LOG=TRACE terraform apply won't print: https://github.com/databricks/databricks-sdk-go/blob/main/config/auth_default.go#L54 on main, prints it after change. Also tested manually for some debug statements.

Ran integration tests and see debug logs.

• make test run locally
• relevant change in docs/ folder
• covered with integration tests in internal/acceptance
• relevant acceptance tests are passing
• using Go SDK

[renaudhartert-db]

I'm not sure I understand what the problem this PR is trying to fix really is. Is it correct that we want the Go SDK to log using the Terraform Logger?

If so, I'm not sure I understand why the Go SDK contexts are problematic. Aren't these given by the terraform provider?

[tanmay-db]

Regarding the points:

1. Yes we want Go SDK to log using Terraform logger (if it is running as part of terraform dependency).
2. Go SDK contexts are problematic as these aren't given by terraform. We define context separately there, for example while Configuring we use refreshCtx that we set earlier as context.Background(): https://github.com/databricks/databricks-sdk-go/blob/main/config/config.go#L365

[renaudhartert-db]

Go SDK contexts are problematic as these aren't given by terraform. We define context separately there, for example while Configuring we use refreshCtx that we set earlier as context.Background(): https://github.com/databricks/databricks-sdk-go/blob/main/config/config.go#L365

Thanks! That is the piece I was missing. We're essentially trying to build a "hack" around a bug in the Go SDK.

I wonder if we could consider an alternative solution that better encapsulate the "hack" so that the code is a little more future proof and less coupled.

My understanding is that the context that is being passed to SetLogger contains all the pieces we need to use. If that is correct, we could leverage that to build a new TfLogger that explicitly encapsulates these pieces. Something like that:

type TfLogger struct {
	name string
	ctx  context.Context
}

func NewTfLogger(ctx context.Context, name string) *TfLogger {
	return &TfLogger{
		name: name,
		ctx:  ctx,
	}
}

func SetLogger(l logger.Logger) {
	logger.DefaultLogger = l
}

// This function is always enabled because TfLogger implements the Logger
// interface from Go SDK and there we check if the logging is enabled based
// on level (which default to Info). This however isn't possible here since
// tflog isn't enabled / disabled based on log level. Omitting is done
// internally through the `ShouldOmit` method that filters based on logger
// configurations.
func (tfl *TfLogger) Enabled(_ context.Context, _ logger.Level) bool {
	return true
}

func (tfl *TfLogger) Tracef(ctx context.Context, format string, v ...any) {
	if tfl.name == "" {
		tflog.Trace(tfl.ctx, fmt.Sprintf(format, v...), nil)
	} else {
		tflog.SubsystemTrace(tfl.ctx, tfl.name, fmt.Sprintf(format, v...), nil)
	}
}

func (tfl *TfLogger) Debugf(ctx context.Context, format string, v ...any) {
	if tfl.name == "" {
		tflog.Debug(tfl.ctx, fmt.Sprintf(format, v...), nil)
	} else {
		tflog.SubsystemDebug(tfl.ctx, tfl.name, fmt.Sprintf(format, v...), nil)
	}
}

func (tfl *TfLogger) Infof(ctx context.Context, format string, v ...any) {
	if tfl.name == "" {
		tflog.Info(tfl.ctx, fmt.Sprintf(format, v...), nil)
	} else {
		tflog.SubsystemInfo(tfl.ctx, tfl.name, fmt.Sprintf(format, v...), nil)
	}
}

func (tfl *TfLogger) Warnf(ctx context.Context, format string, v ...any) {
	if tfl.name == "" {
		tflog.Warn(tfl.ctx, fmt.Sprintf(format, v...), nil)
	} else {
		tflog.SubsystemWarn(tfl.ctx, tfl.name, fmt.Sprintf(format, v...), nil)
	}
}

func (tfl *TfLogger) Errorf(ctx context.Context, format string, v ...any) {
	if tfl.name == "" {
		tflog.Error(tfl.ctx, fmt.Sprintf(format, v...), nil)
	} else {
		tflog.SubsystemError(tfl.ctx, tfl.name, fmt.Sprintf(format, v...), nil)
	}
}

What I like about this solution is that it doesn't rely on any global variable on the Terraform provider side. It also fully commits to a single interface to interact with Go SDK logging (i.e. the DefaultLogger) and literally ignores the context given to the logging methods — which I believe is a great thing as I'm having a hard time seeing the current logger interface remain in SDK V1.

What do you think?

[tanmay-db]

We could have another method such as SetTfLoggerFromContext which will get a new logger and set it but I think it will lead to a loss of readability of code.

SetTfLogger(NewTfLogger(ctx)) clearly say we get a new logger first and set it.

[renaudhartert-db]

[nit] I would just add a small comment here to explain that this function is expecting a certain type of context that contains a logger and not any context.

[tanmay-db]

Added

[renaudhartert-db]

LGTM — thanks for navigating me through the PR Tanmay.

[tanmay-db]

Thanks for the review @renaudhartert-db