Skip to content

Commit 41176a4

Browse files
bfladbflad
authored
tflog+tfsdklog: Change final variadic function parameter in logging functions to ...map[string]interface{} (#34)
Reference: #30 Reference: #31 Previously, implementers could introduce mismatched key-value pairs or expect `f` suffix-like string formatting of the message due to the final `...interface{}` variadic argument. Updating this to `...map[string]interface{}` has the following benefits: - Calls that errantly were treating these as formatter functions will now receive a compiler error in the majority of cases (except parameters that already satisfied the `map[string]interface{}` type). - Matched pairing is now enforced via the compiler, preventing occurence of go-hclog's `EXTRA_VALUE_AT_END` key in entries. - Keys are now enforced to be `string`, where before they could be defined as other types and beholden to go-hclog's behavior of calling `fmt.Sprintf("%s", key)` resulting in likely unexpected keys for non-string types: ``` true: %!s(bool=true) 123: %!s(int=123) 123.456: %!s(float64=123.456) []string{"oops"}: [oops] MyCustomType (without String() method): {} ``` Some disadvantages of this new approach include: - Additional typing of the `map[string]T{}` for each call. Implementors can opt for `map[string]string` in many cases, or Go 1.18+ `map[string]any`, in preference over the potentially confusing/annoying `interface{}` typing. - The burden of converting existing calls. Pragmatically however, the advantages outweigh the disadvantages as this Go module does not yet have full compatibility promises (pre-1.0) and the direct dependencies are expected to grow exponentially in the near future when its existance is broadcasted more. Catching this critical API change earlier rather than later will have less of an effect on the ecosystem.
1 parent 329a757 commit 41176a4

File tree

15 files changed

+1216
-616
lines changed

15 files changed

+1216
-616
lines changed

‎.changelog/34.txt‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
```release-note:breaking-change
2+
tflog: The `Trace()`, `Debug()`, `Info()`, `Warn()`, and `Error()` functions and `Subsystem` equivalents now use `...map[string]interface{}` as the final optional parameter, where the `string` is the structured logging key, rather than expecting matched `key interface{}, value interface{}` pairs. If multiple maps contain the same key, the value is shallow merged.
3+
```
4+
5+
```release-note:breaking-change
6+
tfsdklog: The `Trace()`, `Debug()`, `Info()`, `Warn()`, and `Error()` functions and `Subsystem` equivalents now use `...map[string]interface{}` as the final optional parameter, where the `string` is the structured logging key, rather than expecting matched `key interface{}, value interface{}` pairs. If multiple maps contain the same key, the value is shallow merged.
7+
```

‎internal/hclogutils/args.go‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
package hclogutils
2+
3+
// MapsToArgs will shallow merge field maps into the slice of key1, value1,
4+
// key2, value2, ... arguments expected by hc-log.Logger methods.
5+
func MapsToArgs(maps ...map[string]interface{}) []interface{} {
6+
switch len(maps) {
7+
case 0:
8+
return nil
9+
case 1:
10+
result := make([]interface{}, 0, len(maps[0])*2)
11+
12+
for k, v := range maps[0] {
13+
result = append(result, k)
14+
result = append(result, v)
15+
}
16+
17+
return result
18+
default:
19+
mergedMap := make(map[string]interface{}, 0)
20+
21+
for _, m := range maps {
22+
for k, v := range m {
23+
mergedMap[k] = v
24+
}
25+
}
26+
27+
result := make([]interface{}, 0, len(mergedMap)*2)
28+
29+
for k, v := range mergedMap {
30+
result = append(result, k)
31+
result = append(result, v)
32+
}
33+
34+
return result
35+
}
36+
}

‎internal/hclogutils/args_test.go‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
package hclogutils_test
2+
3+
import (
4+
"sort"
5+
"testing"
6+
7+
"github.com/google/go-cmp/cmp"
8+
"github.com/hashicorp/terraform-plugin-log/internal/hclogutils"
9+
)
10+
11+
func TestMapsToArgs(t *testing.T) {
12+
t.Parallel()
13+
14+
testCases := map[string]struct {
15+
maps []map[string]interface{}
16+
expectedArgs []interface{}
17+
}{
18+
"nil": {
19+
maps: nil,
20+
expectedArgs: nil,
21+
},
22+
"map-single": {
23+
maps: []map[string]interface{}{
24+
{
25+
"map1-key1": "map1-value1",
26+
"map1-key2": "map1-value2",
27+
"map1-key3": "map1-value3",
28+
},
29+
},
30+
expectedArgs: []interface{}{
31+
"map1-key1", "map1-value1",
32+
"map1-key2", "map1-value2",
33+
"map1-key3", "map1-value3",
34+
},
35+
},
36+
"map-multiple-different-keys": {
37+
maps: []map[string]interface{}{
38+
{
39+
"map1-key1": "map1-value1",
40+
"map1-key2": "map1-value2",
41+
"map1-key3": "map1-value3",
42+
},
43+
{
44+
"map2-key1": "map2-value1",
45+
"map2-key2": "map2-value2",
46+
"map2-key3": "map2-value3",
47+
},
48+
},
49+
expectedArgs: []interface{}{
50+
"map1-key1", "map1-value1",
51+
"map1-key2", "map1-value2",
52+
"map1-key3", "map1-value3",
53+
"map2-key1", "map2-value1",
54+
"map2-key2", "map2-value2",
55+
"map2-key3", "map2-value3",
56+
},
57+
},
58+
"map-multiple-mixed-keys": {
59+
maps: []map[string]interface{}{
60+
{
61+
"key1": "map1-value1",
62+
"key2": "map1-value2",
63+
"key3": "map1-value3",
64+
},
65+
{
66+
"key4": "map2-value4",
67+
"key1": "map2-value1",
68+
"key5": "map2-value5",
69+
},
70+
},
71+
expectedArgs: []interface{}{
72+
"key1", "map2-value1",
73+
"key2", "map1-value2",
74+
"key3", "map1-value3",
75+
"key4", "map2-value4",
76+
"key5", "map2-value5",
77+
},
78+
},
79+
"map-multiple-overlapping-keys": {
80+
maps: []map[string]interface{}{
81+
{
82+
"key1": "map1-value1",
83+
"key2": "map1-value2",
84+
"key3": "map1-value3",
85+
},
86+
{
87+
"key1": "map2-value1",
88+
"key2": "map2-value2",
89+
"key3": "map2-value3",
90+
},
91+
},
92+
expectedArgs: []interface{}{
93+
"key1", "map2-value1",
94+
"key2", "map2-value2",
95+
"key3", "map2-value3",
96+
},
97+
},
98+
"map-multiple-overlapping-keys-shallow": {
99+
maps: []map[string]interface{}{
100+
{
101+
"key1": map[string]interface{}{
102+
"submap-key1": "map1-value1",
103+
"submap-key2": "map1-value2",
104+
"submap-key3": "map1-value3",
105+
},
106+
"key2": "map1-value2",
107+
"key3": "map1-value3",
108+
},
109+
{
110+
"key1": map[string]interface{}{
111+
"submap-key4": "map2-value4",
112+
"submap-key5": "map2-value5",
113+
"submap-key6": "map2-value6",
114+
},
115+
"key2": "map2-value2",
116+
"key3": "map2-value3",
117+
},
118+
},
119+
expectedArgs: []interface{}{
120+
"key1", map[string]interface{}{
121+
"submap-key4": "map2-value4",
122+
"submap-key5": "map2-value5",
123+
"submap-key6": "map2-value6",
124+
},
125+
"key2", "map2-value2",
126+
"key3", "map2-value3",
127+
},
128+
},
129+
}
130+
131+
for name, testCase := range testCases {
132+
name, testCase := name, testCase
133+
134+
t.Run(name, func(t *testing.T) {
135+
t.Parallel()
136+
137+
got := hclogutils.MapsToArgs(testCase.maps...)
138+
139+
if len(got)%2 != 0 {
140+
t.Fatalf("expected even number of key-value fields, got: %v", got)
141+
}
142+
143+
if got == nil && testCase.expectedArgs == nil {
144+
return // sortedGot will return []interface{}{} below, nil is what we want
145+
}
146+
147+
// Map retrieval is indeterminate in Go, sort the result first.
148+
// This logic is only necessary in this testing as its automatically
149+
// handled in go-hclog.
150+
gotKeys := make([]string, 0, len(got)/2)
151+
gotValues := make(map[string]interface{}, len(got)/2)
152+
153+
for i := 0; i < len(got); i += 2 {
154+
k, v := got[i].(string), got[i+1]
155+
gotKeys = append(gotKeys, k)
156+
gotValues[k] = v
157+
}
158+
159+
sort.Strings(gotKeys)
160+
161+
sortedGot := make([]interface{}, 0, len(got))
162+
163+
for _, k := range gotKeys {
164+
sortedGot = append(sortedGot, k)
165+
sortedGot = append(sortedGot, gotValues[k])
166+
}
167+
168+
if diff := cmp.Diff(sortedGot, testCase.expectedArgs); diff != "" {
169+
t.Errorf("unexpected difference: %s", diff)
170+
}
171+
})
172+
}
173+
}

‎tflog/provider.go‎

Lines changed: 31 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package tflog
33
import (
44
"context"
55

6+
"github.com/hashicorp/terraform-plugin-log/internal/hclogutils"
67
"github.com/hashicorp/terraform-plugin-log/internal/logging"
78
)
89

@@ -21,10 +22,11 @@ func With(ctx context.Context, key string, value interface{}) context.Context {
2122
return logging.SetProviderRootLogger(ctx, logger.With(key, value))
2223
}
2324

24-
// Trace logs `msg` at the trace level to the logger in `ctx`, with `args` as
25-
// structured arguments in the log output. `args` is expected to be pairs of
26-
// key and value.
27-
func Trace(ctx context.Context, msg string, args ...interface{}) {
25+
// Trace logs `msg` at the trace level to the logger in `ctx`, with optional
26+
// `additionalFields` structured key-value fields in the log output. Fields are
27+
// shallow merged with any defined on the logger, e.g. by the `With()` function,
28+
// and across multiple maps.
29+
func Trace(ctx context.Context, msg string, additionalFields ...map[string]interface{}) {
2830
logger := logging.GetProviderRootLogger(ctx)
2931
if logger == nil {
3032
// this essentially should never happen in production
@@ -34,13 +36,14 @@ func Trace(ctx context.Context, msg string, args ...interface{}) {
3436
// so just making this a no-op is fine
3537
return
3638
}
37-
logger.Trace(msg, args...)
39+
logger.Trace(msg, hclogutils.MapsToArgs(additionalFields...)...)
3840
}
3941

40-
// Debug logs `msg` at the debug level to the logger in `ctx`, with `args` as
41-
// structured arguments in the log output. `args` is expected to be pairs of
42-
// key and value.
43-
func Debug(ctx context.Context, msg string, args ...interface{}) {
42+
// Debug logs `msg` at the debug level to the logger in `ctx`, with optional
43+
// `additionalFields` structured key-value fields in the log output. Fields are
44+
// shallow merged with any defined on the logger, e.g. by the `With()` function,
45+
// and across multiple maps.
46+
func Debug(ctx context.Context, msg string, additionalFields ...map[string]interface{}) {
4447
logger := logging.GetProviderRootLogger(ctx)
4548
if logger == nil {
4649
// this essentially should never happen in production
@@ -50,13 +53,14 @@ func Debug(ctx context.Context, msg string, args ...interface{}) {
5053
// so just making this a no-op is fine
5154
return
5255
}
53-
logger.Debug(msg, args...)
56+
logger.Debug(msg, hclogutils.MapsToArgs(additionalFields...)...)
5457
}
5558

56-
// Info logs `msg` at the info level to the logger in `ctx`, with `args` as
57-
// structured arguments in the log output. `args` is expected to be pairs of
58-
// key and value.
59-
func Info(ctx context.Context, msg string, args ...interface{}) {
59+
// Info logs `msg` at the info level to the logger in `ctx`, with optional
60+
// `additionalFields` structured key-value fields in the log output. Fields are
61+
// shallow merged with any defined on the logger, e.g. by the `With()` function,
62+
// and across multiple maps.
63+
func Info(ctx context.Context, msg string, additionalFields ...map[string]interface{}) {
6064
logger := logging.GetProviderRootLogger(ctx)
6165
if logger == nil {
6266
// this essentially should never happen in production
@@ -66,13 +70,14 @@ func Info(ctx context.Context, msg string, args ...interface{}) {
6670
// so just making this a no-op is fine
6771
return
6872
}
69-
logger.Info(msg, args...)
73+
logger.Info(msg, hclogutils.MapsToArgs(additionalFields...)...)
7074
}
7175

72-
// Warn logs `msg` at the warn level to the logger in `ctx`, with `args` as
73-
// structured arguments in the log output. `args` is expected to be pairs of
74-
// key and value.
75-
func Warn(ctx context.Context, msg string, args ...interface{}) {
76+
// Warn logs `msg` at the warn level to the logger in `ctx`, with optional
77+
// `additionalFields` structured key-value fields in the log output. Fields are
78+
// shallow merged with any defined on the logger, e.g. by the `With()` function,
79+
// and across multiple maps.
80+
func Warn(ctx context.Context, msg string, additionalFields ...map[string]interface{}) {
7681
logger := logging.GetProviderRootLogger(ctx)
7782
if logger == nil {
7883
// this essentially should never happen in production
@@ -82,13 +87,14 @@ func Warn(ctx context.Context, msg string, args ...interface{}) {
8287
// so just making this a no-op is fine
8388
return
8489
}
85-
logger.Warn(msg, args...)
90+
logger.Warn(msg, hclogutils.MapsToArgs(additionalFields...)...)
8691
}
8792

88-
// Error logs `msg` at the error level to the logger in `ctx`, with `args` as
89-
// structured arguments in the log output. `args` is expected to be pairs of
90-
// key and value.
91-
func Error(ctx context.Context, msg string, args ...interface{}) {
93+
// Error logs `msg` at the error level to the logger in `ctx`, with optional
94+
// `additionalFields` structured key-value fields in the log output. Fields are
95+
// shallow merged with any defined on the logger, e.g. by the `With()` function,
96+
// and across multiple maps.
97+
func Error(ctx context.Context, msg string, additionalFields ...map[string]interface{}) {
9298
logger := logging.GetProviderRootLogger(ctx)
9399
if logger == nil {
94100
// this essentially should never happen in production
@@ -98,5 +104,5 @@ func Error(ctx context.Context, msg string, args ...interface{}) {
98104
// so just making this a no-op is fine
99105
return
100106
}
101-
logger.Error(msg, args...)
107+
logger.Error(msg, hclogutils.MapsToArgs(additionalFields...)...)
102108
}

‎tflog/provider_example_test.go‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,10 @@ func ExampleTrace() {
4747
exampleCtx := getExampleContext()
4848

4949
// non-example-setup code begins here
50-
Trace(exampleCtx, "hello, world", "foo", 123, "colors", []string{"red", "blue", "green"})
50+
Trace(exampleCtx, "hello, world", map[string]interface{}{
51+
"foo": 123,
52+
"colors": []string{"red", "blue", "green"},
53+
})
5154

5255
// Output:
5356
// {"@level":"trace","@message":"hello, world","@module":"provider","colors":["red","blue","green"],"foo":123}
@@ -64,7 +67,10 @@ func ExampleDebug() {
6467
exampleCtx := getExampleContext()
6568

6669
// non-example-setup code begins here
67-
Debug(exampleCtx, "hello, world", "foo", 123, "colors", []string{"red", "blue", "green"})
70+
Debug(exampleCtx, "hello, world", map[string]interface{}{
71+
"foo": 123,
72+
"colors": []string{"red", "blue", "green"},
73+
})
6874

6975
// Output:
7076
// {"@level":"debug","@message":"hello, world","@module":"provider","colors":["red","blue","green"],"foo":123}
@@ -81,7 +87,10 @@ func ExampleInfo() {
8187
exampleCtx := getExampleContext()
8288

8389
// non-example-setup code begins here
84-
Info(exampleCtx, "hello, world", "foo", 123, "colors", []string{"red", "blue", "green"})
90+
Info(exampleCtx, "hello, world", map[string]interface{}{
91+
"foo": 123,
92+
"colors": []string{"red", "blue", "green"},
93+
})
8594

8695
// Output:
8796
// {"@level":"info","@message":"hello, world","@module":"provider","colors":["red","blue","green"],"foo":123}
@@ -98,7 +107,10 @@ func ExampleWarn() {
98107
exampleCtx := getExampleContext()
99108

100109
// non-example-setup code begins here
101-
Warn(exampleCtx, "hello, world", "foo", 123, "colors", []string{"red", "blue", "green"})
110+
Warn(exampleCtx, "hello, world", map[string]interface{}{
111+
"foo": 123,
112+
"colors": []string{"red", "blue", "green"},
113+
})
102114

103115
// Output:
104116
// {"@level":"warn","@message":"hello, world","@module":"provider","colors":["red","blue","green"],"foo":123}
@@ -115,7 +127,10 @@ func ExampleError() {
115127
exampleCtx := getExampleContext()
116128

117129
// non-example-setup code begins here
118-
Error(exampleCtx, "hello, world", "foo", 123, "colors", []string{"red", "blue", "green"})
130+
Error(exampleCtx, "hello, world", map[string]interface{}{
131+
"foo": 123,
132+
"colors": []string{"red", "blue", "green"},
133+
})
119134

120135
// Output:
121136
// {"@level":"error","@message":"hello, world","@module":"provider","colors":["red","blue","green"],"foo":123}

0 commit comments

Comments
 (0)
⚡