Conversation
…unctions to ...map[string]interface{}
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.
detro
left a comment
detro
left a comment
There was a problem hiding this comment.
A couple of nitpicking comments (naming and optimising a bit allocation), but I'm a big fan of this refactoring.
Logging, especially when the call stack gets deep, should always be enriched with context. And key/value pairs work great for this.
| mergedMap := make(map[string]interface{}, 0) | ||
|
|
||
| for _, m := range maps { | ||
| for k, v := range m { | ||
| mergedMap[k] = v | ||
| } | ||
| } | ||
|
|
||
| result := make([]interface{}, 0, len(mergedMap)*2) |
There was a problem hiding this comment.
Correct if I'm wrong: what you are really after here (to feed to the second call to make() is for the sum of the number of keys in each map. So the creation of the intermediary mergedMap feels unnecessary memory allocation that will be thrown away when we get out of here.
It is a nitpick, where probably allocating and deallocating a map ain't a bit deal, but I feel Id be remiss if I didn't suggest a cheaper approach:
- count the sum of keys
- loop over all maps to add all their keys to the
result
There was a problem hiding this comment.
The intermediate map performs key deduplication, otherwise, yeah we would just loop through all the maps to append to a single slice. Refer also to the unit tests such as map-multiple-mixed-keys and map-multiple-overlapping-keys.
There was a problem hiding this comment.
Ah, I see what you are doing.
| Trace(exampleCtx, "hello, world", map[string]interface{}{ | ||
| "foo": 123, | ||
| "colors": []string{"red", "blue", "green"}, | ||
| }) |
There was a problem hiding this comment.
Funny enough, I created a similar construct in my last gig: it was Java Exception but the idea was the same. Exceptions happening deep in a call stack would carry extra context in the form of Map.of(PAIRS...) and then New Relic APM would render them as additional context in the error reporting tab.
So, I'm a big fan of this refactoring you are doing.
|
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. |
2 participants
Closes #30
Closes #31
Previously, implementers could introduce mismatched key-value pairs or expect
fsuffix-like string formatting of the message due to the final...interface{}variadic argument. Updating this to...map[string]interface{}has the following benefits:map[string]interface{}type).EXTRA_VALUE_AT_ENDkey in entries.string, where before they could be defined as other types and beholden to go-hclog's behavior of callingfmt.Sprintf("%s", key)resulting in likely unexpected keys for non-string types:Some disadvantages of this new approach include:
map[string]T{}in many calls. Implementors can opt formap[string]stringin many cases, or Go 1.18+map[string]any, in preference over the potentially confusing/annoyinginterface{}typing.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 existence is broadcasted more. Catching this critical API change earlier rather than later will have less of an effect on the ecosystem.