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.

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

Conversation

jakobreinwald
Copy link
Contributor

@jakobreinwald jakobreinwald commented Aug 9, 2024

These changes allow for the processor to gracefully skip log lines that contain no printable characters instead of throwing an error. By returning a different type of error when the message body is empty and no profiles are matched, we can return a warning when processing the empty log line instead of throwing an error with a stack trace and record dump. This change will make it clear to users of the collector that there is not actually an error, but that the log line was empty.

Changes are validated with the addition of the file match_profile_test.go, which contains a unit test to test the various error and success paths in the MatchProfile function.

@b3arpsl b3arpsl self-requested a review August 21, 2024 20:22
Makefile Outdated Show resolved Hide resolved
sllogformatprocessor/batch_logs.go Outdated Show resolved Hide resolved
sllogformatprocessor/batch_logs.go Outdated Show resolved Hide resolved
@@ -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. ")

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.

@@ -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

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.

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

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.

Copy link

@shanemalachow-SL shanemalachow-SL left a comment

Choose a reason for hiding this comment

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

This is looking good now! From a functional perspective, I think this is fine. The code coverage for unit tests is a little lacking. We should ideally test the error paths as well, like https://github.com/ScienceLogic/otel-components/pull/242/files/a4a3612c68f1755b24b74bc1277fd948b0ff2f9c..e1f81913bcf6971535c0820a9341fa3207a8dcf7#diff-f9f640c1a548440250208d7c90a781f27a34d1e2422977452691de378ab141c2R408

Also from a nit standpoint, the tests should probably go into a match_profile_test.go file and not in config_test.go.

That should be enough to make this work.

@shanemalachow-SL shanemalachow-SL self-requested a review September 17, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants