improvement: Rework options

Splits options into logger-level and message-level options.

Semver: Patch
Ref: LOG-7386

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)
 }
 

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)

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

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)
 			}
 		})
 	}

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
 }