tflog+tfsdklog: Added WithAdditionalLocationOffset function
#36
Conversation
Reference: #29 Allows implementations to fix the location offset of caller information when using helper functions. While typically expected for subsystem loggers, SDKs could also offer the configuration of the root provider logger as well. Since each `hclog.Logger` is mainly immutable once its created (except for level and creating named sub-loggers), the `hclog.LoggerOptions` are saved to the context as well, allowing `NewSubsystem` to appropriately have all the root logger options available to create new sub-loggers. Maybe this can be changed upstream in the future, although that would technically be a breaking change since the interface would need to be modified. This changeset also includes setting the name of the test root SDK/provider loggers, so that subsystem naming (and therefore the unit testing) matches real world usage.
bflad
force-pushed
the
bflad-AdditionalLocationOffset
branch
from
1df84e1
to
05b0844
Compare
tflog/provider_test.go
Outdated
| @@ -29,6 +30,7 @@ func TestWith(t *testing.T) { | |||
| { | |||
| "@level": hclog.Trace.String(), | |||
| "@message": "test message", | |||
| "@module": logging.DefaultProviderRootLoggerName, | |||
There was a problem hiding this comment.
I'm from the "school of thought" that test fixtures should be hardcoded and explicit.
IMHO here you should have the same string "provider" duplicated a bunch of times, because using the const itself is helping reducing the chance of the test breaking if, for example, one introduces a typo in the value of DefaultProviderRootLoggerName above.
tflog/subsystem_test.go
Outdated
| const testSubsystem = "test_subsystem" | ||
| const ( | ||
| testSubsystem = "test_subsystem" | ||
| testSubsystemModule = logging.DefaultProviderRootLoggerName + "." + testSubsystem |
There was a problem hiding this comment.
Same point about fixtures as above.
| @@ -31,7 +35,7 @@ func TestSubsystemWith(t *testing.T) { | |||
| { | |||
| "@level": hclog.Trace.String(), | |||
| "@message": "test message", | |||
| "@module": testSubsystem, | |||
| "@module": testSubsystemModule, | |||
There was a problem hiding this comment.
And for all of the @module in this test file
There was a problem hiding this comment.
Updated the constant to include the hardcoded root logger name.
tfsdklog/sdk_test.go
Outdated
| @@ -29,6 +30,7 @@ func TestWith(t *testing.T) { | |||
| { | |||
| "@level": hclog.Trace.String(), | |||
| "@message": "test message", | |||
| "@module": logging.DefaultSDKRootLoggerName, | |||
| @@ -31,7 +35,7 @@ func TestSubsystemWith(t *testing.T) { | |||
| { | |||
| "@level": hclog.Trace.String(), | |||
| "@message": "test message", | |||
| "@module": testSubsystem, | |||
| "@module": testSubsystemModule, | |||
There was a problem hiding this comment.
Updated the constant as noted previously.
tfsdklog/subsystem_test.go
Outdated
| const testSubsystem = "test_subsystem" | ||
| const ( | ||
| testSubsystem = "test_subsystem" | ||
| testSubsystemModule = logging.DefaultSDKRootLoggerName + "." + testSubsystem |
| }, | ||
| expectedOutput: []map[string]interface{}{ | ||
| { | ||
| "@caller": "/tflog/options_test.go:30", |
There was a problem hiding this comment.
I think it would be beneficial for future contributors to know how the integer/line is determined.
I can see someone coming here, changing something like an import or adding a test case above or whatever, and seeing that suddenly all those tests are failing.
A note above the line that warns that the :line part of that string is dependent on the very content of the file is in, it can save someone a bit of head scratching.
| // Caller line (number after colon) should match | ||
| // tflog.SubsystemTrace() line in test case implementation. |
| @@ -29,6 +29,7 @@ func TestWith(t *testing.T) { | |||
| { | |||
| "@level": hclog.Trace.String(), | |||
| "@message": "test message", | |||
| "@module": "provider", | |||
|
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. |
Closes #29
Allows implementations to fix the location offset of caller information when using helper functions. While typically expected for subsystem loggers, SDKs could also offer the configuration of the root provider logger as well.
Since each
hclog.Loggeris mainly immutable once its created (except for level and creating named sub-loggers), thehclog.LoggerOptionsare saved to the context as well, allowingNewSubsystemto appropriately have all the root logger options available to create new sub-loggers. Maybe this can be changed upstream in the future, although that would technically be a breaking change since the interface would need to be modified.This changeset also includes setting the name of the test root SDK/provider loggers, so that subsystem naming (and therefore the unit testing) matches real world usage.