Streamline error logging #2403
Streamline error logging #2403
Conversation
| func (p templateEventPrinter) Print(event trace.Event) { | ||
| if p.templateObj != nil { | ||
| err := (*p.templateObj).Execute(p.out, event) | ||
| if err != nil { | ||
| p.Error(err) | ||
| logger.Error("error executing tempalte", err) |
There was a problem hiding this comment.
typo: template
logger, besides the first arg, requires key/value pairs, so here we can pick one of options:
logger.Error("executing template", "error", err) // kinda redundant
logger.Error("executing template - " + err.Error())
logger.Error(fmt.Sprintf("executing template %v", err))
I really don't have a hard opinion about it.
There was a problem hiding this comment.
But we have to standardize it for our own good.
There was a problem hiding this comment.
I'll fix it, but I must say it's a bit unintuitive to work like this with implicit key values, since the usage of the function does not imply it at all.
There was a problem hiding this comment.
It's the way structured loggers like zap work.
But logrus has a different API:
Anyway our API could easily be changed to:
logger.Error("executing template", err)
...
func Error(msg string, err error, keysAndValues ...interface{}) {
errorw(1, pkgLogger, fmt.Sprintf("%s - %s", msg, err.Error()), keysAndValues...)
}In this case, if there's no error, we must use logger.Info()/Warn() etc.
There was a problem hiding this comment.
In zap afaik you do zap.Int(field, intVal) which is self documenting unlike what we do now where we implicitly need to know how the api works.
Anyway I fixed the logs to align with it.
There was a problem hiding this comment.
It (typed pair) was initially proposed but the use of unspecified types (through interfaces) was picked in the merged PR. I think that were comments asking to make the API cleaner (simple).
Anyway I fixed the logs to align with it.
👍🏼
There was a problem hiding this comment.
@NDStrahilevitz Do you think it's worth opening a discussion about the logger api?
| @@ -283,6 +283,21 @@ func (l *Logger) Sync() error { | |||
| return l.l.Sync() | |||
| } | |||
|
|
|||
| // Setters | |||
| // TODO: add SetWriter and Setlogger to the struct level? | |||
There was a problem hiding this comment.
logger offers two ways of logging... by the package level and by receiver level. In the tracee we are only using the pkg level. For the sake of compliance I would add the option for receivers. If in the future we are not going to use different loggers (instantiations), we can keep the pkg level by just removing the rest.
There was a problem hiding this comment.
My initial solution was first implementing the receiver then using it globally, however it didn't work as expected. I don't think we actually use the receiver level at all right? Maybe we should consider removing it entirely.
(note: trivy also uses a similar strategy with only a global logger they initialize once with an InitGlobal like method. It would simplify our logger a bit to be similar to that).
There was a problem hiding this comment.
I don't think we actually use the receiver level at all right? Maybe we should consider removing it entirely.
I agree. I just implemented it as an option since we didn't have a specific goal that time. So let's remove the receiver option.
(note: trivy also uses a similar strategy with only a global logger they initialize once with an InitGlobal like method. It would simplify our logger a bit to be similar to that).
Alright.
cmd/tracee-ebpf/main.go
Outdated
| @@ -148,7 +148,8 @@ func main() { | |||
| } | |||
|
|
|||
| printerConfig.ContainerMode = containerMode | |||
| cfg.Output = &output | |||
| cfg.Output = &output.OutputConfig | |||
| logger.SetWriter(output.LogFile) | |||
There was a problem hiding this comment.
Is this the unique point of configuration? Wouldn't it be required to set it through env vars?
There was a problem hiding this comment.
Programmatically we can set it through SetBase.
From my experience working on this package now I think we should consider moving away from all the implicit env vars, since it made logic much more complex with a single global Init() point of entry.
There was a problem hiding this comment.
Programmatically we can set it through SetBase. From my experience working on this package now I think we should consider moving away from all the implicit env vars, since it made logic much more complex with a single global Init() point of entry.
Should env vars be checked before the logger.Init() (outside the logger)?
There was a problem hiding this comment.
I think we should drop the env vars, it wasn't a good design decision in hindsight, library consumers should Init the logger on their own if they want non default behavior (this is what we do in other aqua products too).
|
@NDStrahilevitz made an issue for this - #2404. |
ac09915 to
75dd957
Compare
|
@rafaeldtinoco had to run format fixes since lint wasn't passing for some reason. |
917d7d1 to
7fe3b53
Compare
|
@NDStrahilevitz patch: diff --git a/pkg/cmd/flags/flags_test.go b/pkg/cmd/flags/flags_test.go
index e8197cae..e5b87a0e 100644
--- a/pkg/cmd/flags/flags_test.go
+++ b/pkg/cmd/flags/flags_test.go
@@ -103,8 +103,8 @@ func TestPrepareFilter(t *testing.T) {
expectedError: filters.InvalidValue("-1"),
},
{
- testName: "invalid uid 1",
- filters: []string{"uid= "},
+ testName: "invalid uid 1",
+ filters: []string{"uid= "},
expectedError: filters.InvalidValue("\t"),
},
{ |
e14f270 to
33dfb68
Compare
|
@yanivagman @rafaeldtinoco style tests (and normal) are passing now, your reviews would be appreciated |
docs/docs/tracing/output-formats.md
Outdated
|
|
||
| ```text | ||
| $ sudo TRACEE_BPF_FILE=do-not-exist ./dist/tracee-ebpf --output json --trace comm=bash --trace follow --trace event=openat --output out-file:/tmp/tracee.log --output err-file:/tmp/tracee.err | ||
| $ sudo TRACEE_BPF_FILE=do-not-exist ./dist/tracee-ebpf --output json --trace comm=bash --trace follow --trace event=openat --output out-file:/tmp/tracee.log --output log-file:/tmp/tracee.log |
There was a problem hiding this comment.
tracee.log is used in this example both for out-file and log-file. Better change one of them to something else
There was a problem hiding this comment.
tracee.events and tracee.log respectively?
There was a problem hiding this comment.
tracee.eventsandtracee.logrespectively?
Can be. The main point is to have different files
pkg/cmd/flags/output.go
Outdated
| @@ -19,7 +19,7 @@ Possible options: | |||
| [format:]gob output events in gob format | |||
| [format:]gotemplate=/path/to/template output events formatted using a given gotemplate file | |||
| out-file:/path/to/file write the output to a specified file. create/trim the file if exists (default: stdout) | |||
| err-file:/path/to/file write the errors to a specified file. create/trim the file if exists (default: stderr) | |||
| log-file:/path/to/file write the errors to a specified file. create/trim the file if exists (default: stderr) | |||
There was a problem hiding this comment.
comment should be changed from write the errors... to write the logs...
c7f9aa1 to
e30df10
Compare
|
@yanivagman fixed the docs issues you pointed out, are we good to go with this? |
tracee.handleError redirects errors to the logger instead of an error channel. Additionally, the printer's error method is also removed, using the logger for printing errors too.
Change the err-file flag to log-file, which handles redirecting tracee-ebpf's log output.
e30df10 to
50ca8ad
Compare
Due ChanErrors removal aquasecurity#2403, warn about it.
Due ChanErrors removal aquasecurity#2403, warn about it.
Due ChanErrors removal aquasecurity#2403, warn about it.
Due ChanErrors removal aquasecurity#2403, warn about it.
Due ChanErrors removal aquasecurity#2403, warn about it.
Due ChanErrors removal aquasecurity#2403, warn about it.
Due ChanErrors removal aquasecurity#2403, warn about it.
Due ChanErrors removal aquasecurity#2403, warn about it.
* logger: ebpf errors Introduce EBPF error control mechanism - submission via perf buffer. EBPF errors are aggregated before being submitted and are later processed and logged by the processBPFErrors() go routine. This starts the use of bpf error mechanism through some type checking like BPF_ERR_MAP_UPDATE_ELEM, BPF_ERR_GET_CURRENT_COMM and BPF_ERR_INIT_CONTEXT. * logger: warn event loss Due ChanErrors removal #2403, warn about it. * pkg/ebpf: make string copy work in focal 4.19
Initial Checklist
Description (git log)
commit 4fcc473
commit 94efde2
Fixes: #2404
Type of change
How Has This Been Tested?
tracee-ebpf -o log-file:test
Final Checklist:
Pick "Bug Fix" or "Feature", delete the other and mark appropriate checks.
Git Log Checklist:
My commits logs have: