Skip to content

integrate logrus logging to rego interpreter package - #1730

Open
Maksim An (anmaxvl) wants to merge 1 commit into
microsoft:mainfrom
anmaxvl:rego-interpreter-logging
Open

integrate logrus logging to rego interpreter package#1730
Maksim An (anmaxvl) wants to merge 1 commit into
microsoft:mainfrom
anmaxvl:rego-interpreter-logging

Conversation

@anmaxvl

Copy link
Copy Markdown
Contributor

Add a new InterpreterLogger interface and introduce 2 implementations: fileLogger and logrusLogger. File logger behaves the same way as the previous implementation, except that it uses logrus, rather than go built-in. Logrus logger implementation uses hcsshim's logrus wrappers.

@anmaxvl
Maksim An (anmaxvl) requested a review from a team as a code owner April 14, 2023 04:14
@anmaxvl
Maksim An (anmaxvl) force-pushed the rego-interpreter-logging branch 2 times, most recently from afc087a to 9e77b73 Compare April 14, 2023 04:34
@matajoh

Copy link
Copy Markdown
Contributor

Looks good, thanks for doing this work!

@helsaawy Hamza El-Saawy (helsaawy) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the logger interface
probably something we should do for the whole repo so we can replace logrus with zap or something a lot easier

return LogLevel(atomic.LoadInt32((*int32)(&ll.level)))
}

type InterpreterLogger interface {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

theres a structured logger thats being added to the std library that defines this and has a bunch of convince functions:
https://pkg.go.dev/golang.org/x/exp/slog#Handler

i think youd just need to define an implementation for the handler, then the slog.New(...) should work and give you all the logging functions you need

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

looked more into this. problem is that interpreter defines rather different "levels" and we may need to decide how we want to map them? In general slog seems like a good replacement for logrus.

Comment thread pkg/securitypolicy/regopolicy_test.go Outdated
var _ InterpreterLogger = (*logrusLogger)(nil)

func NewFileLogger(path string, level LogLevel) (InterpreterLogger, error) {
file, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0666)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it make sense to set the log file to be 0644 so that it can only be written to by the owning user?

@anmaxvl
Maksim An (anmaxvl) force-pushed the rego-interpreter-logging branch from 9e77b73 to ba10efd Compare May 9, 2023 10:33
Add a new InterpreterLogger interface and introduce 2 implementations:
fileLogger and logrusLogger. File logger behaves the same way as the
previous implementation, except that it uses logrus, rather than go built-in.
Logrus logger implementation uses hcsshim's logrus wrappers.

Signed-off-by: Maksim An <maksiman@microsoft.com>
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.

6 participants