From 2883a7987b22d1809bf4ff1ebab73d60d2788a42 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 | 12 ++-- logger/logger_test.go | 20 +++---- logger/options.go | 130 +++++++++++++++++++++++++++++++++-------- logger/options_test.go | 35 +++++------ logger/transport.go | 10 ---- test.go | 39 +++++++++++++ 6 files changed, 178 insertions(+), 68 deletions(-) create mode 100644 test.go diff --git a/logger/logger.go b/logger/logger.go index fe038b2..aff2588 100644 --- a/logger/logger.go +++ b/logger/logger.go @@ -8,15 +8,14 @@ import ( // 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 + Options MessageOptions } // Payload contains the properties sent to the ingestion endpoint. @@ -84,16 +83,17 @@ func (l *Logger) Close() { // Log sends a provided log message to LogDNA. func (l *Logger) Log(message string) { + msgOpts := MessageOptions{App: l.Options.App, Env: l.Options.Env, FlushInterval: l.Options.FlushInterval, SendTimeout: l.Options.SendTimeout, Hostname: l.Options.Hostname, IndexMeta: l.Options.IndexMeta, IngestURL: l.Options.IngestURL, IPAddress: l.Options.IPAddress, Level: l.Options.Level, MacAddress: l.Options.MacAddress, MaxBufferLen: l.Options.MaxBufferLen, Meta: l.Options.Meta, Tags: l.Options.Tags} logMsg := Message{ Body: message, - Options: l.Options, + Options: 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 { +func (l *Logger) LogWithOptions(message string, options MessageOptions) error { msgOpts := l.Options.merge(options) err := msgOpts.validate() if err != nil { @@ -111,7 +111,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{Level: level} return l.LogWithOptions(message, options) } diff --git a/logger/logger_test.go b/logger/logger_test.go index 0002ec8..d262ba3 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,16 +103,16 @@ 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", + l.LogWithOptions("testing", MessageOptions{ + App: "anotherapp", + Env: "production", + Level: "error", + Timestamp: time.Now(), }) l.Close() @@ -138,8 +138,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{ + App: strings.Repeat("a", 83), }) assert.Error(t, err) diff --git a/logger/options.go b/logger/options.go index 8ea99fa..bda449c 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,50 +37,131 @@ type Options struct { MaxBufferLen int Meta string Tags string +} + +// MessageOptions encapsulates user-provided options such as the Level and App +// that are passed along with each log. +type MessageOptions struct { + App string + Env string + FlushInterval time.Duration + SendTimeout time.Duration + Hostname string + IndexMeta bool + IngestURL string + IPAddress string + Level string + MacAddress string + MaxBufferLen int + Meta string + Tags string Timestamp time.Time } -func (e InvalidOptionMessage) String() string { - return fmt.Sprintf("Options.%s: %s", e.Option, e.Message) +type fieldIssue struct { + field string + prob 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()) +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) *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 != "" && net.ParseIP(options.IPAddress) == nil { + issues = append(issues, fieldIssue{"IPAddress", "Invalid format"}) } - if options.IPAddress != "" && !reIPAddress.MatchString(options.IPAddress) { - problems = append(problems, InvalidOptionMessage{"IPAddress", "invalid format"}.String()) + if len(issues) > 0 { + return &optionsError{issues: issues} } + return nil +} + +func (options *MessageOptions) validate() error { + 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])`) + + var issues []fieldIssue - if len(problems) > 0 { - return errors.New(strings.Join(problems, ", ")) + 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) { + issues = append(issues, fieldIssue{"Hostname", "Invalid format"}) + } + if options.IPAddress != "" && net.ParseIP(options.IPAddress) == nil { + issues = append(issues, fieldIssue{"IPAddress", "Invalid format"}) } + if len(issues) > 0 { + return &optionsError{issues: issues} + } return nil } -func (options Options) merge(merge Options) Options { +func (options Options) merge(merge MessageOptions) MessageOptions { newOpts := options if merge.App != "" { newOpts.App = merge.App @@ -94,8 +175,7 @@ func (options Options) merge(merge Options) Options { if merge.Meta != "" { newOpts.Meta = merge.Meta } - - return newOpts + return MessageOptions{App: newOpts.App, Env: newOpts.Env, FlushInterval: newOpts.FlushInterval, SendTimeout: newOpts.SendTimeout, Hostname: newOpts.Hostname, IndexMeta: newOpts.IndexMeta, IngestURL: newOpts.IngestURL, IPAddress: newOpts.IPAddress, Level: newOpts.Level, MacAddress: newOpts.MacAddress, MaxBufferLen: newOpts.MaxBufferLen, Meta: newOpts.Meta, Tags: newOpts.Tags, Timestamp: merge.Timestamp} } func (options *Options) setDefaults() { diff --git a/logger/options_test.go b/logger/options_test.go index 01c85ee..d9fce3a 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) } }) } @@ -45,17 +45,18 @@ func TestOptions_Merge(t *testing.T) { Meta: `{"foo": "bar"}`, } - o = o.merge(Options{ + newOptions := MessageOptions{} + newOptions = o.merge(MessageOptions{ App: "merge", Env: "merge", Level: "merge", Meta: `{"baz": "merge"}`, }) - assert.Equal(t, "merge", o.App) - assert.Equal(t, "merge", o.Env) - assert.Equal(t, "merge", o.Level) - assert.Equal(t, `{"baz": "merge"}`, o.Meta) + assert.Equal(t, "merge", newOptions.App) + assert.Equal(t, "merge", newOptions.Env) + assert.Equal(t, "merge", newOptions.Level) + assert.Equal(t, `{"baz": "merge"}`, newOptions.Meta) } func TestOptions_SetDefaults(t *testing.T) { diff --git a/logger/transport.go b/logger/transport.go index 5b04713..59b04d0 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() }() @@ -145,13 +143,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 } diff --git a/test.go b/test.go new file mode 100644 index 0000000..02e4509 --- /dev/null +++ b/test.go @@ -0,0 +1,39 @@ +package main + +import ( + "fmt" + "time" + + "github.com/logdna/logdna-go/logger" +) + +func main() { + key := "aebc17b5be5a2ad7fad7e0423cbd46b2" + + options := logger.Options{} + options.Level = "fatal" + options.Hostname = "gotest" + options.App = "test" + options.IPAddress = "10.0.1.101" + options.MacAddress = "c0:ff:ee:c0:ff:ee" + options.Env = "production" + options.Tags = "logging,golang" + + myLogger, err := logger.NewLogger(options, key) + fmt.Println(err) + // myLogger.Log("Message 1") + // myLogger.Log("Message 1") + // myLogger.Log("Message 1") + // myLogger.Log("Message 1") + // myLogger.Log("Message 1") + // myLogger.Log("Message 1") + // myLogger.Log("Message 1") + // myLogger.Log("Message 1") + // myLogger.Log("Message 1") + // myLogger.Log("Message 1") + // myLogger.Log("Message 1") + // myLogger.Log("Message 1") + msgOpts := logger.MessageOptions{Timestamp: time.Now()} + myLogger.LogWithOptions("Message 2", msgOpts) + myLogger.Close() +}