Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tflog+tfsdklog: Change final variadic function parameter in logging functions to ...map[string]interface{} #34

Merged
merged 5 commits into from
Mar 8, 2022

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Mar 4, 2022

Closes #30
Closes #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 occurrence 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{} in many calls. 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 existence is broadcasted more. Catching this critical API change earlier rather than later will have less of an effect on the ecosystem.

…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.
@bflad bflad added the bug Something isn't working label Mar 4, 2022
@bflad bflad added this to the v0.3.0 milestone Mar 4, 2022
@bflad bflad requested a review from a team as a code owner March 4, 2022 21:11
Copy link
Contributor

@detro detro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +19 to +27
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you are doing.

tflog/provider.go Outdated Show resolved Hide resolved
Comment on lines +50 to +53
Trace(exampleCtx, "hello, world", map[string]interface{}{
"foo": 123,
"colors": []string{"red", "blue", "green"},
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bflad bflad merged commit 41176a4 into main Mar 8, 2022
@bflad bflad deleted the bflad-map-parameters branch March 8, 2022 18:23
@github-actions
Copy link

github-actions bot commented Apr 8, 2022

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
2 participants