From 8c1a1254b064fa41434e6084229cb76fb94ae704 Mon Sep 17 00:00:00 2001 From: Dhruv Madhok Date: Thu, 24 Sep 2020 22:44:10 -0700 Subject: [PATCH] improvement: Rework options Splits options into logger-level and message-level options. Semver: Patch Ref: LOG-7386 --- logger/logger.go | 23 ++++++------ logger/logger_test.go | 18 ++++------ logger/options.go | 82 ++++++++++++++++++++++++++++-------------- logger/options_test.go | 24 ++++++------- logger/transport.go | 27 ++++---------- 5 files changed, 93 insertions(+), 81 deletions(-) diff --git a/logger/logger.go b/logger/logger.go index fe038b2..08c4918 100644 --- a/logger/logger.go +++ b/logger/logger.go @@ -2,21 +2,21 @@ package logger import ( "encoding/json" + "time" "github.com/joho/godotenv" ) // Logger is the means by which a user can begin to send logs. type Logger struct { - Options Options - + Options Options transport *transport } // Message represents a single log message and associated options. type Message struct { - Body string - Options Options + Body string + MessageOptions MessageOptions } // Payload contains the properties sent to the ingestion endpoint. @@ -84,25 +84,26 @@ func (l *Logger) Close() { // Log sends a provided log message to LogDNA. func (l *Logger) Log(message string) { + msgOpts := MessageOptions{l.Options, time.Now()} logMsg := Message{ - Body: message, - Options: l.Options, + Body: message, + MessageOptions: msgOpts, } l.transport.add(logMsg) } // LogWithOptions allows the user to update options uniquely for a given log message // before sending the log to LogDNA. -func (l *Logger) LogWithOptions(message string, options Options) error { - msgOpts := l.Options.merge(options) +func (l *Logger) LogWithOptions(message string, options MessageOptions) error { + msgOpts := l.Options.merge(options.Options) err := msgOpts.validate() if err != nil { return err } logMsg := Message{ - Body: message, - Options: msgOpts, + Body: message, + MessageOptions: MessageOptions{msgOpts, time.Now()}, } l.transport.add(logMsg) @@ -111,7 +112,7 @@ func (l *Logger) LogWithOptions(message string, options Options) error { // LogWithLevel sends a log message to LogDNA with a parameterized level. func (l *Logger) LogWithLevel(message string, level string) error { - options := Options{Level: level} + options := MessageOptions{Options{Level: level}, time.Now()} return l.LogWithOptions(message, options) } diff --git a/logger/logger_test.go b/logger/logger_test.go index 0002ec8..53babc3 100644 --- a/logger/logger_test.go +++ b/logger/logger_test.go @@ -34,7 +34,7 @@ func TestLogger_NewLogger(t *testing.T) { t.Run("Invalid options", func(t *testing.T) { o := Options{ - Level: strings.Repeat("a", 33), + Level: strings.Repeat("a", 83), } _, err := NewLogger(o, "abc123") @@ -58,7 +58,7 @@ func TestLogger_Log(t *testing.T) { Hostname: "foo", App: "test", IPAddress: "127.0.0.1", - MacAddress: "C0:FF:EE:C0:FF:EE", + MacAddress: "c0:ff:ee:c0:ff:ee", } l, err := NewLogger(o, "abc123") @@ -71,7 +71,7 @@ func TestLogger_Log(t *testing.T) { assert.Equal(t, "abc123", body["apikey"]) assert.Equal(t, "foo", body["hostname"]) assert.Equal(t, "127.0.0.1", body["ip"]) - assert.Equal(t, "C0:FF:EE:C0:FF:EE", body["mac"]) + assert.Equal(t, "c0:ff:ee:c0:ff:ee", body["mac"]) assert.NotEmpty(t, body["lines"]) ls := body["lines"].([]interface{}) @@ -103,17 +103,13 @@ func TestLogger_LogWithOptions(t *testing.T) { App: "app", Env: "development", Level: "info", - Timestamp: now, } l, err := NewLogger(o, "abc123") assert.Equal(t, nil, err) - l.LogWithOptions("testing", Options{ - App: "anotherapp", - Env: "production", - Level: "error", - }) + options := Options{App: "anotherapp", Env: "production", Level: "error"} + l.LogWithOptions("testing", MessageOptions{options, time.Now()}) l.Close() assert.NotEmpty(t, body) @@ -138,8 +134,8 @@ func TestLogger_LogWithOptions(t *testing.T) { l, err := NewLogger(o, "abc123") assert.Equal(t, nil, err) - err = l.LogWithOptions("testing", Options{ - App: strings.Repeat("a", 33), + err = l.LogWithOptions("testing", MessageOptions{ + Options{App: strings.Repeat("a", 83)}, time.Now(), }) assert.Error(t, err) diff --git a/logger/options.go b/logger/options.go index 8ea99fa..3588939 100644 --- a/logger/options.go +++ b/logger/options.go @@ -1,8 +1,8 @@ package logger import ( - "errors" "fmt" + "net" "regexp" "strings" "time" @@ -13,6 +13,7 @@ const ( defaultSendTimeout = 30 * time.Second defaultFlushInterval = 250 * time.Millisecond defaultMaxBufferLen = 50 + maxOptionLength = 80 ) // InvalidOptionMessage represents an issue with the supplied configuration. @@ -21,8 +22,7 @@ type InvalidOptionMessage struct { Message string } -// Options encapsulates user-provided options such as the Level and App -// that are passed along with each log. +// Options encapsulates user-provided options. type Options struct { App string Env string @@ -37,46 +37,76 @@ type Options struct { MaxBufferLen int Meta string Tags string - Timestamp time.Time } -func (e InvalidOptionMessage) String() string { - return fmt.Sprintf("Options.%s: %s", e.Option, e.Message) +// MessageOptions encapsulates user-provided options such as the Level and App +// that are passed along with each log. +type MessageOptions struct { + Options + Timestamp time.Time +} + +type fieldIssue struct { + field string + prob string +} + +type optionsError struct { + issues []fieldIssue +} + +func (e *optionsError) Error() string { + var str strings.Builder + str.WriteString("One or more invalid options:\n") + for i := 0; i < len(e.issues); i++ { + str.WriteString(fmt.Sprintf("%s: %s\n", e.issues[i].field, e.issues[i].prob)) + } + return str.String() } -func validateOptionLength(option string, value string, problems *[]string) { - if len(value) > 32 { - *problems = append(*problems, InvalidOptionMessage{option, "length must be less than 32"}.String()) +func validateOptionLength(option string, value string) *fieldIssue { + if len(value) > maxOptionLength { + return &fieldIssue{field: option, prob: "length must be less than 80"} } + return nil } func (options *Options) validate() error { - var problems []string - reMacAddress := regexp.MustCompile(`^([0-9a-fA-F][0-9a-fA-F]:){5}([0-9a-fA-F][0-9a-fA-F])`) reHostname := regexp.MustCompile(`(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9-]*[A-Za-z0-9])`) - reIPAddress := regexp.MustCompile(`(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}`) - validateOptionLength("App", options.App, &problems) - validateOptionLength("Env", options.Env, &problems) - validateOptionLength("Hostname", options.Hostname, &problems) - validateOptionLength("Level", options.Level, &problems) + var issues []fieldIssue - if options.MacAddress != "" && (!reMacAddress.MatchString(options.MacAddress)) { - problems = append(problems, InvalidOptionMessage{"MacAddress", "invalid format"}.String()) + if issue := validateOptionLength("App", options.App); issue != nil { + issues = append(issues, *issue) + } + if issue := validateOptionLength("Env", options.Env); issue != nil { + issues = append(issues, *issue) + } + if issue := validateOptionLength("Hostname", options.Hostname); issue != nil { + issues = append(issues, *issue) + } + if issue := validateOptionLength("Level", options.Level); issue != nil { + issues = append(issues, *issue) } + if options.MacAddress != "" { + mac, err := net.ParseMAC(options.MacAddress) + if err != nil { + issues = append(issues, fieldIssue{"MacAddress", "Invalid format"}) + } else { + options.MacAddress = mac.String() + } + } if options.Hostname != "" && !reHostname.MatchString(options.Hostname) { - problems = append(problems, InvalidOptionMessage{"Hostname", "invalid format"}.String()) + issues = append(issues, fieldIssue{"Hostname", "Invalid format"}) } - - if options.IPAddress != "" && !reIPAddress.MatchString(options.IPAddress) { - problems = append(problems, InvalidOptionMessage{"IPAddress", "invalid format"}.String()) + if options.IPAddress != "" && net.ParseIP(options.IPAddress) == nil { + issues = append(issues, fieldIssue{"IPAddress", "Invalid format"}) } - if len(problems) > 0 { - return errors.New(strings.Join(problems, ", ")) + if len(issues) > 0 { + return &optionsError{issues: issues} } - return nil } @@ -94,10 +124,8 @@ func (options Options) merge(merge Options) Options { if merge.Meta != "" { newOpts.Meta = merge.Meta } - return newOpts } - func (options *Options) setDefaults() { if options.SendTimeout == 0 { options.SendTimeout = defaultSendTimeout diff --git a/logger/options_test.go b/logger/options_test.go index 01c85ee..3a74475 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -12,26 +12,26 @@ func TestOptions_Validate(t *testing.T) { testCases := []struct { label string options Options - valid bool + errStr string }{ - {"Base case", Options{}, true}, - {"With options", Options{Level: "error", Hostname: "foo", App: "app", IPAddress: "127.0.0.1", MacAddress: "C0:FF:EE:C0:FF:EE"}, true}, - {"App length", Options{App: strings.Repeat("a", 33)}, false}, - {"Env length", Options{Env: strings.Repeat("a", 33)}, false}, - {"Hostname length", Options{Hostname: strings.Repeat("a", 33)}, false}, - {"Level length", Options{Level: strings.Repeat("a", 33)}, false}, - {"Invalid MacAddress", Options{MacAddress: "in:va:lid"}, false}, - {"Invalid Hostname", Options{Hostname: "-"}, false}, - {"Invalid IPAddress", Options{IPAddress: "localhost"}, false}, + {"Base case", Options{}, ""}, + {"With options", Options{Level: "error", Hostname: "foo", App: "app", IPAddress: "127.0.0.1", MacAddress: "C0:FF:EE:C0:FF:EE"}, ""}, + {"App length", Options{App: strings.Repeat("a", 83)}, "One or more invalid options:\nApp: length must be less than 80\n"}, + {"Env length", Options{Env: strings.Repeat("a", 83)}, "One or more invalid options:\nEnv: length must be less than 80\n"}, + {"Hostname length", Options{Hostname: strings.Repeat("a", 83)}, "One or more invalid options:\nHostname: length must be less than 80\n"}, + {"Level length", Options{Level: strings.Repeat("a", 83)}, "One or more invalid options:\nLevel: length must be less than 80\n"}, + {"Invalid MacAddress", Options{MacAddress: "in:va:lid"}, "One or more invalid options:\nMacAddress: Invalid format\n"}, + {"Invalid Hostname", Options{Hostname: "-"}, "One or more invalid options:\nHostname: Invalid format\n"}, + {"Invalid IPAddress", Options{IPAddress: "localhost"}, "One or more invalid options:\nIPAddress: Invalid format\n"}, } for _, tc := range testCases { t.Run(tc.label, func(t *testing.T) { err := tc.options.validate() - if tc.valid { + if tc.errStr == "" { assert.Equal(t, nil, err) } else { - assert.Error(t, err) + assert.EqualError(t, err, tc.errStr) } }) } diff --git a/logger/transport.go b/logger/transport.go index 5b04713..3b5f621 100644 --- a/logger/transport.go +++ b/logger/transport.go @@ -81,8 +81,6 @@ func (t *transport) flushSend() { t.wg.Add(1) go func() { - // TODO(mdeltito): in the future a retry should be triggered - // with the msgs pulled out of the buffer t.send(msgs) t.wg.Done() }() @@ -93,21 +91,18 @@ func (t *transport) send(msgs []Message) error { for _, msg := range msgs { line := Line{ Body: msg.Body, - App: msg.Options.App, - Env: msg.Options.Env, - Level: msg.Options.Level, + App: msg.MessageOptions.Options.App, + Env: msg.MessageOptions.Options.Env, + Level: msg.MessageOptions.Options.Level, } - timestamp := msg.Options.Timestamp - if timestamp.IsZero() { - timestamp = time.Now() - } + timestamp := msg.MessageOptions.Timestamp line.Timestamp = timestamp.UnixNano() / int64(time.Millisecond) - if msg.Options.Meta != "" { + if msg.MessageOptions.Options.Meta != "" { line.Meta = metaEnvelope{ - indexed: msg.Options.IndexMeta, - meta: msg.Options.Meta, + indexed: msg.MessageOptions.Options.IndexMeta, + meta: msg.MessageOptions.Options.Meta, } } @@ -145,13 +140,5 @@ func (t *transport) send(msgs []Message) error { return fmt.Errorf("Server error: %d", resp.StatusCode) } - // TODO(mdeltito): do we need to parse the response? - // we arent doing anything with it - var apiresp ingestAPIResponse - err = json.NewDecoder(resp.Body).Decode(&apiresp) - if err != nil { - return err - } - return nil }