Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Rework how we store log fields (#81)
* Introducing `internal/fieldutils` to manipulate what we are calling "field maps"

"Field maps" are `map[string]interface{}`, that carry additional fields we want the logging library to attach to logs

* Removing now unnecessary `htlogutils.ArgsToKeys` function

* Creating new `Fields` options for the `internal/logging` `LoggerOpts` options

This is what gets stored in the context with each log, and will now carry the fields we want to append to logs directly (as a map),
instead of relying on the `hclog` `ImpliedArgs` facility.

* Switch the log filtering functionality to be entirely based on the field maps

* Switching `tflog` and `tfsdklog` to use the new "native" support for fields

This doesn't change the interface of the library, but it changes where the fields are stored before they are logged.

* linting

* Adding `lint`, `fmt` and `test` to makefile

Just shortcuts for go commands
  • Loading branch information
Ivan De Marino committed Jul 15, 2022
1 parent 60509c8 commit a2e3687
Show file tree
Hide file tree
Showing 12 changed files with 554 additions and 515 deletions.
11 changes: 10 additions & 1 deletion Makefile
Expand Up @@ -40,4 +40,13 @@ website/build-local:
@docker build https://github.com/hashicorp/terraform-website.git\#$(WEBSITE_BRANCH) \
-t $(WEBSITE_DOCKER_IMAGE_LOCAL)

.PHONY: website website/local website/build-local
lint:
golangci-lint run

fmt:
gofmt -s -w -e .

test:
go test -v -cover -timeout=120s -parallel=4 ./...

.PHONY: lint fmt test website website/local website/build-local
45 changes: 45 additions & 0 deletions internal/fieldutils/field_maps.go
@@ -0,0 +1,45 @@
package fieldutils

// MergeFieldMaps takes a slice of field maps,
// and merges all the key/value pairs into a new single field map.
//
// Input order matters: in case two or more maps use the same key,
// the last one to set that key will have the corresponding value
// persisted.
func MergeFieldMaps(maps ...map[string]interface{}) map[string]interface{} {
// Pre-allocate a map to merge all the maps into,
// that has at least the capacity equivalent to the number
// of maps to merge
result := make(map[string]interface{}, len(maps))

// Merge all the maps into one;
// in case of clash, only the last key is preserved
for _, m := range maps {
for k, v := range m {
result[k] = v
}
}

return result
}

// FieldMapsToKeys will extract all the field maps keys, avoiding repetitions
// in case two or more maps contained the same key.
func FieldMapsToKeys(maps ...map[string]interface{}) []string {
switch len(maps) {
case 0:
return nil
case 1:
result := make([]string, 0, len(maps[0]))

for k := range maps[0] {
result = append(result, k)
}

return result
default:
// As we merge all maps into one, we can use this
// same function recursively, falling back on the `switch case 1`.
return FieldMapsToKeys(MergeFieldMaps(maps...))
}
}
238 changes: 238 additions & 0 deletions internal/fieldutils/field_maps_test.go
@@ -0,0 +1,238 @@
package fieldutils_test

import (
"sort"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/hashicorp/terraform-plugin-log/internal/fieldutils"
)

func TestMergeFieldMaps(t *testing.T) {
t.Parallel()

testCases := map[string]struct {
input []map[string]interface{}
expected map[string]interface{}
}{
"nil": {
input: nil,
expected: map[string]interface{}{},
},
"empty-maps": {
input: []map[string]interface{}{
{},
{},
{},
},
expected: map[string]interface{}{},
},
"single-map": {
input: []map[string]interface{}{
{
"k1": 123,
"k2": "two",
"k3": 3.45,
},
},
expected: map[string]interface{}{
"k1": 123,
"k2": "two",
"k3": 3.45,
},
},
"multiple-maps": {
input: []map[string]interface{}{
{
"k1": 123,
"k2": "two",
"k3": 3.45,
},
{
"k4": true,
"k5": "five",
"k6": 6.78,
},
},
expected: map[string]interface{}{
"k1": 123,
"k2": "two",
"k3": 3.45,
"k4": true,
"k5": "five",
"k6": 6.78,
},
},
"key-collision": {
input: []map[string]interface{}{
{
"k1": 123,
"k2": "two",
"k3": 3.45,
},
{
"k1": true,
"k5": "five",
"k3": 6.78,
},
{
"k4": "four",
},
},
expected: map[string]interface{}{
"k1": true,
"k2": "two",
"k3": 6.78,
"k5": "five",
"k4": "four",
},
},
}

for name, testCase := range testCases {
name, testCase := name, testCase

t.Run(name, func(t *testing.T) {
t.Parallel()

got := fieldutils.MergeFieldMaps(testCase.input...)

if diff := cmp.Diff(got, testCase.expected); diff != "" {
t.Errorf("unexpected difference: %s", diff)
}
})
}
}

func TestFieldMapsToKeys(t *testing.T) {
t.Parallel()

testCases := map[string]struct {
maps []map[string]interface{}
expected []string
}{
"nil": {
maps: nil,
expected: nil,
},
"map-single": {
maps: []map[string]interface{}{
{
"map1-key1": "map1-value1",
"map1-key2": "map1-value2",
"map1-key3": "map1-value3",
},
},
expected: []string{
"map1-key1",
"map1-key2",
"map1-key3",
},
},
"map-multiple-different-keys": {
maps: []map[string]interface{}{
{
"map1-key1": "map1-value1",
"map1-key2": "map1-value2",
"map1-key3": "map1-value3",
},
{
"map2-key1": "map2-value1",
"map2-key2": "map2-value2",
"map2-key3": "map2-value3",
},
},
expected: []string{
"map1-key1",
"map1-key2",
"map1-key3",
"map2-key1",
"map2-key2",
"map2-key3",
},
},
"map-multiple-mixed-keys": {
maps: []map[string]interface{}{
{
"key1": "map1-value1",
"key2": "map1-value2",
"key3": "map1-value3",
},
{
"key4": "map2-value4",
"key1": "map2-value1",
"key5": "map2-value5",
},
},
expected: []string{
"key1",
"key2",
"key3",
"key4",
"key5",
},
},
"map-multiple-overlapping-keys": {
maps: []map[string]interface{}{
{
"key1": "map1-value1",
"key2": "map1-value2",
"key3": "map1-value3",
},
{
"key1": "map2-value1",
"key2": "map2-value2",
"key3": "map2-value3",
},
},
expected: []string{
"key1",
"key2",
"key3",
},
},
"map-multiple-overlapping-keys-shallow": {
maps: []map[string]interface{}{
{
"key1": map[string]interface{}{
"submap-key1": "map1-value1",
"submap-key2": "map1-value2",
"submap-key3": "map1-value3",
},
"key2": "map1-value2",
"key3": "map1-value3",
},
{
"key1": map[string]interface{}{
"submap-key4": "map2-value4",
"submap-key5": "map2-value5",
"submap-key6": "map2-value6",
},
"key3": "map2-value3",
"key2": "map2-value2",
},
},
expected: []string{
"key1",
"key2",
"key3",
},
},
}

for name, testCase := range testCases {
name, testCase := name, testCase

t.Run(name, func(t *testing.T) {
t.Parallel()

got := fieldutils.FieldMapsToKeys(testCase.maps...)

sort.Strings(got)

if diff := cmp.Diff(got, testCase.expected); diff != "" {
t.Errorf("unexpected difference: %s", diff)
}
})
}
}
49 changes: 6 additions & 43 deletions internal/hclogutils/args.go
@@ -1,12 +1,12 @@
package hclogutils

import (
"fmt"
"github.com/hashicorp/terraform-plugin-log/internal/fieldutils"
)

// MapsToArgs will shallow merge field maps into a slice of key/value pairs
// FieldMapsToArgs will shallow merge field maps into a slice of key/value pairs
// arguments (i.e. `[k1, v1, k2, v2, ...]`) expected by hc-log.Logger methods.
func MapsToArgs(maps ...map[string]interface{}) []interface{} {
func FieldMapsToArgs(maps ...map[string]interface{}) []interface{} {
switch len(maps) {
case 0:
return nil
Expand All @@ -19,45 +19,8 @@ func MapsToArgs(maps ...map[string]interface{}) []interface{} {

return result
default:
// Pre-allocate a map to merge all the maps into,
// that has at least the capacity equivalent to the number
// of maps to merge
mergedMap := make(map[string]interface{}, len(maps))

// Merge all the maps into one;
// in case of clash, only the last key is preserved
for _, m := range maps {
for k, v := range m {
mergedMap[k] = v
}
}

// As we have merged all maps into one, we can use this
// same function recursively for the `switch case 1`.
return MapsToArgs(mergedMap)
}
}

// ArgsToKeys will extract all keys from a slice of key/value pairs
// arguments (i.e. `[k1, v1, k2, v2, ...]`) expected by hc-log.Logger methods.
//
// Note that, in case of an odd number of arguments, the last key captured
// will refer to a value that does not actually exist.
func ArgsToKeys(args []interface{}) []string {
// Pre-allocate enough capacity to fit all the keys,
// i.e. all the elements in the input array in even position
keys := make([]string, 0, len(args)/2)

for i := 0; i < len(args); i += 2 {
// All keys should be strings, but in case they are not
// we format them to string
switch k := args[i].(type) {
case string:
keys = append(keys, k)
default:
keys = append(keys, fmt.Sprintf("%s", k))
}
// As we merge all maps into one, we can use this
// same function recursively, falling back on the `switch case 1`.
return FieldMapsToArgs(fieldutils.MergeFieldMaps(maps...))
}

return keys
}

0 comments on commit a2e3687

Please sign in to comment.