Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[processor/sllogformat] skip with warning for empty log lines #242

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[processor/sllogformat] skip with warning for empty log lines #242

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2e8db56
Add error details to match_profile
Aug 2, 2024
a4a3612
Warnings for unnecessary log lines
Aug 9, 2024
ab23513
Finalized implementation and unit testing
Sep 13, 2024
e1f8191
Added back in govulncheck
Sep 13, 2024
2570133
Made test table-driven, added more cases
Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ all-groups:
@echo "\nall: $(ALL_MODS)"

.PHONY: all
all: install-tools gotidy gofmt gogovulncheck gotest
b3arpsl marked this conversation as resolved.
Show resolved Hide resolved
all: install-tools gotidy gofmt gotest

.PHONY: gotidy
gotidy:
Expand Down
23 changes: 22 additions & 1 deletion sllogformatprocessor/batch_logs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,15 @@ import (
"context"
"crypto/sha1"
"encoding/json"
"errors"
"fmt"
"strings"
"unicode"

"go.uber.org/zap"

"go.opentelemetry.io/collector/consumer"
"go.opentelemetry.io/collector/pdata/pcommon"
"go.opentelemetry.io/collector/pdata/plog"
)

Expand Down Expand Up @@ -100,7 +104,11 @@ func (bl *batchLogs) addToBatch(ld plog.Logs) {
rl.ScopeLogs().RemoveIf(func(ils plog.ScopeLogs) bool {
ils.LogRecords().RemoveIf(func(lr plog.LogRecord) bool {
gen, req, err := bl.cfg.MatchProfile(bl.log, rl, ils, lr)
if err != nil {
if err != nil && errors.Is(err, skipLine) {
b3arpsl marked this conversation as resolved.
Show resolved Hide resolved
bl.log.Warn("Failed to match profile",
zap.String("err", err.Error()))
return true
} else if err != nil {
bl.log.Error("Failed to match profile",
zap.String("err", err.Error()))
bl.dumpLogRecord(rl, ils, lr)
Expand Down Expand Up @@ -151,6 +159,19 @@ func (bl *batchLogs) addToBatch(ld plog.Logs) {
})
}

func AsStringWithoutUnprintables(v pcommon.Value) string {
b3arpsl marked this conversation as resolved.
Show resolved Hide resolved
var ret string = v.AsString()
ret = strings.Map(func(r rune) rune {
if unicode.IsPrint(r) {
return r
} else if r == '\n' {
return ' '
}
return -1
}, ret)
return ret
}

func (bl *batchLogs) dumpLogRecord(rl plog.ResourceLogs, ils plog.ScopeLogs, lr plog.LogRecord) {
buf := dataBuffer{}
buf.logEntry("Resource SchemaURL: %s", rl.SchemaUrl())
Expand Down
16 changes: 14 additions & 2 deletions sllogformatprocessor/match_profile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package sllogformatprocessor // import "github.com/open-telemetry/opentelemetry-
import (
"encoding/json"
"errors"
"fmt"
"regexp"
"strconv"
"strings"
Expand All @@ -27,6 +28,8 @@ import (
"go.uber.org/zap"
)

var skipLine = errors.New("skipping line: message body contains no printable characters; log line is empty. ")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably declare this error as a struct. It allows a bit more flexibility in the future at the cost of a little more code. With this, we get support for errors.Is() and errors.As() as well as enabling us in the future to start printing more verbose errors by providing more context in struct fields (like if we want the error to contain what type of non-printable characters it may have found. Something like this:

type NoPrintablesError struct {}

func (err *NoPrintablesError) Error() string {
    return "lorem ipsum doleret"
}

Secondarily, that error message is a little messy, I would simplify to "log message has no printable characters" (runes if we want to be accurate to Go's representation of strings). Go errors generally need to be concise, and they're traditionally wrapped by prepending the wrapping error context, and then adding a colon to print the suberror. An error like index not found, would be wrapped later by failed to look up row: index not found. So, you normally want to avoid colons and semicolons in your messages and keep them short and sweet.


type StreamTokenReq struct {
Stream string `json:"stream"`
Logbasename string `json:"logbasename"`
Expand Down Expand Up @@ -388,7 +391,8 @@ type ConfigResult struct {

func (c *Config) MatchProfile(log *zap.Logger, rl plog.ResourceLogs, ils plog.ScopeLogs, lr plog.LogRecord) (*ConfigResult, *StreamTokenReq, error) {
var id, ret string
for _, profile := range c.Profiles {
reasons := []string{}
for idx, profile := range c.Profiles {
req := newStreamTokenReq()
gen := ConfigResult{}
parser := Parser{
Expand All @@ -399,16 +403,19 @@ func (c *Config) MatchProfile(log *zap.Logger, rl plog.ResourceLogs, ils plog.Sc
}
id, gen.ServiceGroup = parser.EvalElem(profile.ServiceGroup)
if gen.ServiceGroup == "" {
reasons = append(reasons, "service_group")
continue
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to not check all the other elements as well? As it stands, if ServiceGroup and Host are empty, we won't know about Host until later because we're skipping the rest of the loop.

}
req.Ids[id] = gen.ServiceGroup
id, gen.Host = parser.EvalElem(profile.Host)
if gen.Host == "" {
reasons = append(reasons, "host")
continue
}
req.Ids[id] = gen.Host
id, gen.Logbasename = parser.EvalElem(profile.Logbasename)
if gen.Logbasename == "" {
reasons = append(reasons, "logbasename")
continue
}
if lr.SeverityNumber() == plog.SeverityNumberUnspecified {
Expand All @@ -420,6 +427,7 @@ func (c *Config) MatchProfile(log *zap.Logger, rl plog.ResourceLogs, ils plog.Sc
if profile.Severity != nil {
_, sevText := parser.EvalElem(profile.Severity)
if sevText == "" {
reasons = append(reasons, "severity")
continue
}
sevText = strings.ToUpper(sevText)
Expand Down Expand Up @@ -447,6 +455,10 @@ func (c *Config) MatchProfile(log *zap.Logger, rl plog.ResourceLogs, ils plog.Sc
}
_, gen.Message = parser.EvalElem(profile.Message)
if gen.Message == "" {
if idx >= len(c.Profiles)-1 {
return nil, nil, skipLine
}
reasons = append(reasons, "message")
continue
}
// FORMAT MESSAGE
Expand Down Expand Up @@ -480,5 +492,5 @@ func (c *Config) MatchProfile(log *zap.Logger, rl plog.ResourceLogs, ils plog.Sc
gen.Format = profile.Format
return &gen, &req, nil
}
return nil, nil, errors.New("No matching profile for log record")
return nil, nil, fmt.Errorf("No matching profile for log record, failed to find %v", reasons)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably shorten this a bit, and use strings.Join(reasons, ", ") to make it pretty. Again, we may also want to make this a struct so we can break it down later, even including the slice of reasons directly.

}