From 5e20bc16964c6e2c4481f81147f53df1e6610945 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 16 Mar 2018 08:37:07 +0000 Subject: [PATCH 1/8] On panic while printing, attempt to print panic value This should be more useful to users with bugs in the code they are trying to log from. --- encode.go | 6 +++--- encode_test.go | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/encode.go b/encode.go index 55f1603..ff6f0f1 100644 --- a/encode.go +++ b/encode.go @@ -278,7 +278,7 @@ func safeError(err error) (s string, ok bool) { if v := reflect.ValueOf(err); v.Kind() == reflect.Ptr && v.IsNil() { s, ok = "null", false } else { - panic(panicVal) + s, ok = fmt.Sprintf("PANIC:%v", panicVal), false } } }() @@ -292,7 +292,7 @@ func safeString(str fmt.Stringer) (s string, ok bool) { if v := reflect.ValueOf(str); v.Kind() == reflect.Ptr && v.IsNil() { s, ok = "null", false } else { - panic(panicVal) + s, ok = fmt.Sprintf("PANIC:%v", panicVal), true } } }() @@ -306,7 +306,7 @@ func safeMarshal(tm encoding.TextMarshaler) (b []byte, err error) { if v := reflect.ValueOf(tm); v.Kind() == reflect.Ptr && v.IsNil() { b, err = nil, nil } else { - panic(panicVal) + b, err = nil, fmt.Errorf("panic when marshalling: %s", panicVal) } } }() diff --git a/encode_test.go b/encode_test.go index ebebaae..4b6144b 100644 --- a/encode_test.go +++ b/encode_test.go @@ -125,6 +125,10 @@ func TestMarshalKeyvals(t *testing.T) { {in: kv(decimalStringer{5, 9}, "v"), want: []byte("5.9=v")}, {in: kv((*decimalStringer)(nil), "v"), err: logfmt.ErrNilKey}, {in: kv(marshalerStringer{5, 9}, "v"), want: []byte("5.9=v")}, + {in: kv("k", panicingStringer{0}), want: []byte("k=ok")}, + {in: kv("k", panicingStringer{1}), want: []byte("k=PANIC:panic1")}, + // Need extra mechanism to test panic-while-printing-panicVal + //{in: kv("k", panicingStringer{2}), want: []byte("?")}, } for _, d := range data { @@ -196,6 +200,20 @@ func (errorMarshaler) MarshalText() ([]byte, error) { return nil, errMarshal } +type panicingStringer struct { + a int +} + +func (p panicingStringer) String() string { + switch p.a { + case 1: + panic("panic1") + case 2: + panic(panicingStringer{a: 2}) + } + return "ok" +} + func BenchmarkEncodeKeyval(b *testing.B) { b.ReportAllocs() enc := logfmt.NewEncoder(ioutil.Discard) From c2152ce9154221b192a66372715a5330c67dbce3 Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Fri, 27 Apr 2018 17:21:01 -0400 Subject: [PATCH 2/8] Update Go versions for travis.ci builds. --- .travis.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index b599f65..9cdc1d0 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,11 +1,11 @@ language: go sudo: false go: - - 1.3 - - 1.4 - - 1.5 - - 1.6 - - tip + - "1.7" + - "1.8" + - "1.9" + - "1.10" + - "tip" before_install: - go get github.com/mattn/goveralls From 11fd56866eade1ecd7cd56144e08d5d50669fd48 Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Tue, 20 Nov 2018 01:57:09 -0500 Subject: [PATCH 3/8] Drop invalid runes from keys instead of returning ErrInvalidKey Experience has shown that including invalid runes (especially spaces) in keys is the most common mistake when encoding. Returning an error for keys containing invalid runes is not helpful because log packages often discard the field or entire record when there is an encoding error. But missing log data surprises application developers that don't understand why it was discarded. I considered two ways to handle bad runes in keys, optionally replacing them with a different configurable rune, or dropping the bad runes from they key. The first approach requires expanding the API and adding some complexity to the implementation of Encoder. The second choice does not change the API and, thanks to the strings.Map and bytes.Map functions, slightly simplifies the implementation; so I chose it. --- encode.go | 33 +++++++++--------- encode_internal_test.go | 76 +++++++++++++++++++++++++++++++---------- 2 files changed, 75 insertions(+), 34 deletions(-) diff --git a/encode.go b/encode.go index ff6f0f1..4ea9d23 100644 --- a/encode.go +++ b/encode.go @@ -110,8 +110,8 @@ func (e *MarshalerError) Error() string { // a nil interface or pointer value. var ErrNilKey = errors.New("nil key") -// ErrInvalidKey is returned by Marshal functions and Encoder methods if a key -// contains an invalid character. +// ErrInvalidKey is returned by Marshal functions and Encoder methods if, after +// dropping invalid runes, a key is empty. var ErrInvalidKey = errors.New("invalid key") // ErrUnsupportedKeyType is returned by Encoder methods if a key has an @@ -165,31 +165,32 @@ func writeKey(w io.Writer, key interface{}) error { } } -func invalidKeyRune(r rune) bool { - return r <= ' ' || r == '=' || r == '"' || r == utf8.RuneError -} - -func invalidKeyString(key string) bool { - return len(key) == 0 || strings.IndexFunc(key, invalidKeyRune) != -1 -} - -func invalidKey(key []byte) bool { - return len(key) == 0 || bytes.IndexFunc(key, invalidKeyRune) != -1 +// keyRuneFilter returns r for all valid key runes, and -1 for all invalid key +// runes. When used as the mapping function for strings.Map and bytes.Map +// functions it causes them to remove invalid key runes from strings or byte +// slices respectively. +func keyRuneFilter(r rune) rune { + if r <= ' ' || r == '=' || r == '"' || r == utf8.RuneError { + return -1 + } + return r } func writeStringKey(w io.Writer, key string) error { - if invalidKeyString(key) { + k := strings.Map(keyRuneFilter, key) + if k == "" { return ErrInvalidKey } - _, err := io.WriteString(w, key) + _, err := io.WriteString(w, k) return err } func writeBytesKey(w io.Writer, key []byte) error { - if invalidKey(key) { + k := bytes.Map(keyRuneFilter, key) + if len(k) == 0 { return ErrInvalidKey } - _, err := w.Write(key) + _, err := w.Write(k) return err } diff --git a/encode_internal_test.go b/encode_internal_test.go index 6271ce8..926c201 100644 --- a/encode_internal_test.go +++ b/encode_internal_test.go @@ -4,6 +4,7 @@ import ( "bytes" "errors" "fmt" + "io/ioutil" "reflect" "testing" ) @@ -26,11 +27,26 @@ func TestSafeMarshal(t *testing.T) { } func TestWriteKeyStrings(t *testing.T) { - keygen := []func(string) interface{}{ - func(s string) interface{} { return s }, - func(s string) interface{} { return stringData(s) }, - func(s string) interface{} { return stringStringer(s) }, - func(s string) interface{} { return stringMarshaler(s) }, + keygen := []struct { + name string + fn func(string) interface{} + }{ + { + name: "string", + fn: func(s string) interface{} { return s }, + }, + { + name: "named-string", + fn: func(s string) interface{} { return stringData(s) }, + }, + { + name: "Stringer", + fn: func(s string) interface{} { return stringStringer(s) }, + }, + { + name: "TextMarshaler", + fn: func(s string) interface{} { return stringMarshaler(s) }, + }, } data := []struct { @@ -48,23 +64,30 @@ func TestWriteKeyStrings(t *testing.T) { {key: " ", err: ErrInvalidKey}, {key: "=", err: ErrInvalidKey}, {key: `"`, err: ErrInvalidKey}, + {key: "k\n", want: "k"}, + {key: "k\nk", want: "kk"}, + {key: "k\tk", want: "kk"}, + {key: "k=k", want: "kk"}, + {key: `"kk"`, want: "kk"}, } for _, g := range keygen { - for _, d := range data { - w := &bytes.Buffer{} - key := g(d.key) - err := writeKey(w, key) - if err != d.err { - t.Errorf("%#v (%[1]T): got error: %v, want error: %v", key, err, d.err) + t.Run(g.name, func(t *testing.T) { + for _, d := range data { + w := &bytes.Buffer{} + key := g.fn(d.key) + err := writeKey(w, key) + if err != d.err { + t.Errorf("%#v: got error: %v, want error: %v", key, err, d.err) + } + if err != nil { + continue + } + if got, want := w.String(), d.want; got != want { + t.Errorf("%#v: got '%s', want '%s'", key, got, want) + } } - if err != nil { - continue - } - if got, want := w.String(), d.want; got != want { - t.Errorf("%#v (%[1]T): got '%s', want '%s'", key, got, want) - } - } + }) } } @@ -231,3 +254,20 @@ type errorMarshaler struct{} func (errorMarshaler) MarshalText() ([]byte, error) { return nil, errMarshaling } + +func BenchmarkWriteStringKey(b *testing.B) { + keys := []string{ + "k", + "caller", + "has space", + `"quoted"`, + } + + for _, k := range keys { + b.Run(k, func(b *testing.B) { + for i := 0; i < b.N; i++ { + writeStringKey(ioutil.Discard, k) + } + }) + } +} From d4c98a4d6da3fd8382bd0b43d4f66678eaa36c64 Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Tue, 20 Nov 2018 02:13:11 -0500 Subject: [PATCH 4/8] Update Go versions for travis.ci builds --- .travis.yml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index 9cdc1d0..25976da 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,10 +1,11 @@ language: go sudo: false go: - - "1.7" - - "1.8" - - "1.9" - - "1.10" + - "1.7.x" + - "1.8.x" + - "1.9.x" + - "1.10.x" + - "1.11.x" - "tip" before_install: From 576c7c8c2256303ea77f864b63dae78c0157b434 Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Wed, 21 Nov 2018 20:20:54 -0500 Subject: [PATCH 5/8] Add CHANGELOG.md --- CHANGELOG.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..1826c4b --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,23 @@ +# Changelog +All notable changes to this project will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [Unreleased] + +## 0.3.0 - 2016-11-15 +### Added +- Pool buffers for quoted strings and byte slices by [@nussjustin](https://github.com/nussjustin) +### Fixed +- Fuzz fix, quote invalid UTF-8 values by [@judwhite](https://github.com/judwhite) + +## 0.2.0 - 2016-05-08 +### Added +- Encoder.EncodeKeyvals + +## 0.1.0 - 2016-03-28 +### Added +- Encoder +- Decoder +- MarshalKeyvals From f2ca038802095b8a357ba8800e5e2e6aa1168c78 Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Wed, 21 Nov 2018 20:44:26 -0500 Subject: [PATCH 6/8] Add unreleased changes, contributors, and links --- CHANGELOG.md | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1826c4b..aa2039a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,18 +6,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -## 0.3.0 - 2016-11-15 +### Changed +- Drop invalid runes from keys instead of returning ErrInvalidKey by [@ChrisHines] +- On panic while printing, attempt to print panic value by [@bboreham] + +## [0.3.0] - 2016-11-15 ### Added -- Pool buffers for quoted strings and byte slices by [@nussjustin](https://github.com/nussjustin) +- Pool buffers for quoted strings and byte slices by [@nussjustin] ### Fixed -- Fuzz fix, quote invalid UTF-8 values by [@judwhite](https://github.com/judwhite) +- Fuzz fix, quote invalid UTF-8 values by [@judwhite] -## 0.2.0 - 2016-05-08 +## [0.2.0] - 2016-05-08 ### Added -- Encoder.EncodeKeyvals +- Encoder.EncodeKeyvals by [@ChrisHines] -## 0.1.0 - 2016-03-28 +## [0.1.0] - 2016-03-28 ### Added -- Encoder -- Decoder -- MarshalKeyvals +- Encoder by [@ChrisHines] +- Decoder by [@ChrisHines] +- MarshalKeyvals by [@ChrisHines] + +[0.3.0]: https://github.com/go-logfmt/logfmt/compare/v0.2.0...v0.3.0 +[0.2.0]: https://github.com/go-logfmt/logfmt/compare/v0.1.0...v0.2.0 +[0.1.0]: https://github.com/go-logfmt/logfmt/commits/v0.1.0 + +[@ChrisHines]: https://github.com/ChrisHines +[@bboreham]: https://github.com/bboreham +[@judwhite]: https://github.com/judwhite +[@nussjustin]: https://github.com/nussjustin From f183dd30c31ddf0b350d70e241e2072e15553ddb Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Wed, 21 Nov 2018 20:53:00 -0500 Subject: [PATCH 7/8] Support go modules --- CHANGELOG.md | 3 +++ go.mod | 3 +++ go.sum | 2 ++ 3 files changed, 8 insertions(+) create mode 100644 go.mod create mode 100644 go.sum diff --git a/CHANGELOG.md b/CHANGELOG.md index aa2039a..f1f8000 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- Go module support by [@ChrisHines] + ### Changed - Drop invalid runes from keys instead of returning ErrInvalidKey by [@ChrisHines] - On panic while printing, attempt to print panic value by [@bboreham] diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..63d50f0 --- /dev/null +++ b/go.mod @@ -0,0 +1,3 @@ +module github.com/go-logfmt/logfmt + +require github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515 diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..d0cdc16 --- /dev/null +++ b/go.sum @@ -0,0 +1,2 @@ +github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515 h1:T+h1c/A9Gawja4Y9mFVWj2vyii2bbUNDw3kt9VxK2EY= +github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515/go.mod h1:+0opPa2QZZtGFBFZlji/RkVcI2GknAs/DXo4wKdlNEc= From 0e6ce8488fbaa80902e6d747c39bb5446ead3f41 Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Wed, 21 Nov 2018 20:55:41 -0500 Subject: [PATCH 8/8] Update CHANGELOG --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1f8000..3455b8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,10 +4,11 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [Unreleased] +## [0.4.0] - 2018-11-21 ### Added - Go module support by [@ChrisHines] +- CHANGELOG by [@ChrisHines] ### Changed - Drop invalid runes from keys instead of returning ErrInvalidKey by [@ChrisHines] @@ -29,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Decoder by [@ChrisHines] - MarshalKeyvals by [@ChrisHines] +[0.4.0]: https://github.com/go-logfmt/logfmt/compare/v0.3.0...v0.4.0 [0.3.0]: https://github.com/go-logfmt/logfmt/compare/v0.2.0...v0.3.0 [0.2.0]: https://github.com/go-logfmt/logfmt/compare/v0.1.0...v0.2.0 [0.1.0]: https://github.com/go-logfmt/logfmt/commits/v0.1.0