Bootstrap a basic logger wrapper #2
Conversation
Testing with timestamps is annoying. Let's add an option only this package can use to turn them off.
When creating a new logger context, disable the time and change the output if those options are set. Also, default to TRACE if no level is set. When instantiating a subsystem logger, honor the level that's set on it. Remove the example from the GoDoc string for all our methods, as the actual examples that will be executed as part of Go test will handle providing examples of usage that is more likely to stay correct over time.
Add examples that run as part of `go test` to show how the tflog package is meant to be used.
Instead of using a var, we can use a const, as this isn't supposed to change.
Bring over the changes from tflog to tfsdklog.
Make the name of the SDK logger overridable. If no level is set for the SDK logger, default to TRACE. Respect the options to disable timestamps and to set the output when constructing new SDK loggers.
|
I think this is basically ready for review. :) |
|
Here's a preview of what this looks like on pkg.go.dev, complete with examples and GoDoc. |
go-plugin's Serve function overwrites os.Stderr with a writer that gets sent over a gRPC stream. This is great, except currently, Terraform doesn't actually read from that stream at all. While the ideal fix here is for Terraform to read from that stream the way it currently does stderr for the provider process, the current behavior exists, and that means we need to work around it. The workaround in this case is to copy os.Stderr at startup, before Serve gets a chance to overwrite it, and write to that instead of the overwritten os.Stderr. This mimics the behavior in SDKv2's helper/logging package. This isn't ideal long-term behavior, because it's subverting go-plugin's expectations of what's writing to stderr and when, so it's hidden behind an Option so it's opt-in. That way we can deprecate and remove the option at some point in the future, when Terraform has started reading from the gRPC stream that go-plugin provides and enough practitioners have upgraded to that version of Terraform.
| // Option defines a modification to the configuration for a logger. | ||
| type Option func(loggerOpts) loggerOpts |
There was a problem hiding this comment.
Given the only difference between the root package Options type and the sdk package Option type is the applyLoggerOpts implementation to include the sdk name default, should this type and its receiver methods be removed in preference of using the root Option type? The New() and NewSubsystem() functions can and should remain separate to apply those defaults in this package except the parameters become tflog.Option. I do not necessarily think it is a bad thing if the sdk sub-package depends on importing the root package and it will help ensure we are providing any necessary options for all loggers in one implementation.
Alternatively, we could alias all the Option handling in this package to the root package, but I think the above is preferable to just remove the duplicate code.
There was a problem hiding this comment.
In order to share these between the two packages, we'd need to export loggerOpts and all the properties on it, and I think I'd prefer not to do that. I think the duplication here isn't bad, and a little copying may be preferable to a little dependency. Especially if we want to introduce options that only apply to one of these logging contexts.
There was a problem hiding this comment.
Another option would be to move that to an exported Go type in an internal/ package. Not the biggest deal though, in my opinion. My initial reaction when reviewing this was "what is different between the two Option implementations" but sounds like there is a future use case.
There was a problem hiding this comment.
Yeah, I think that's a potential future direction. I think my justification here is that though they look the same now, the audiences are different, and so the options may differ in the future. For example, setting the output isn't something provider devs should really worry about, but it may make sense for SDK devs.
But then again, both uses of options are really meant so SDK devs, so I don't know.
I think, given that it's a private, internal-only detail, it's probably fine for the moment, because refactoring will have basically no user-visible changes, so we're always able to refactor it into an internal package later.
Use the newly released hclog options to correct locations that are printed with logs to not point to plugin-log but to whatever called it.
|
Updated with the comments from @bflad's review. New pkg.go.dev. |
|
|
||
| // WithLevelFromEnv returns an option that will set the level of the logger | ||
| // based on the string in an environment variable. The environment variable | ||
| // checked will be `name` and `subsystems`, joined by _ and in all caps. |
There was a problem hiding this comment.
Good call, I'll follow up with another PR for that and some README/doc.go tweaks as part of release prep.
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Start to flesh out the implementation of this module.