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

Add full file path to error logs that contain a stack #65

Open
akajla09 opened this issue Apr 7, 2023 · 4 comments
Open

Add full file path to error logs that contain a stack #65

akajla09 opened this issue Apr 7, 2023 · 4 comments
Labels
good first issue Good for newcomers

Comments

@akajla09
Copy link
Member

akajla09 commented Apr 7, 2023

Is your feature request related to a problem? Please describe.
Warrant uses zerolog for structured logging (with stack enabled) to make debugging errors easier. Currently, zerolog only prints the filename (e.g. sqlite.go or repository.go) in the stack without the full file path which makes it difficult to quickly figure out which pkg the file belongs to, given that multiple Warrant packages have similar filenames (repository.go etc). For example, it's hard to tell at a first glance from the following log which sqlite.go repository is causing an error:

stack=[{"func":"SQL.ExecContext","line":"225","source":"sql.go"},{"func":"SQLiteRepository.DeleteByFeatureId","line":"252","source":"sqlite.go"}]

Describe the solution you'd like
There should be a configuration in zerolog to add the full path of the filename (including pkg subdir) so we can print /authz/object/sqlite.go vs. sqlite.go in the logs.

Note: depending on implementation and computational complexity, we might want to make this setting configurable so users can disable in prod if desired.

Additional context
https://github.com/rs/zerolog#add-file-and-line-number-to-log

@akajla09 akajla09 added the good first issue Good for newcomers label Apr 7, 2023
@AKARSHITJOSHI
Copy link

AKARSHITJOSHI commented Apr 8, 2023

Hi @akajla09 I would like to contribute on this.
My proposed solution would be to change log.Logger = log.Output(zerolog.ConsoleWriter{Out: os.Stderr})
in config.go to log.With().Caller().Logger().Output(zerolog.ConsoleWriter{Out: os.Stderr}).
Let me know what you think about this.
PS: Thanks

@akajla09
Copy link
Member Author

akajla09 commented Apr 8, 2023

Hi @AKARSHITJOSHI - thanks for offering to help. I haven't looked into this deeply so can't say for certain if this change will work. Have you tried it locally and tested it out? If you have a sample log output from the change, we can look at it to see if it contains the full filepath.

@AKARSHITJOSHI
Copy link

Please have a look at the following image
Screenshot 2023-04-09 at 11 06 34 AM

Message :
{"level":"fatal","error":"While parsing config: yaml: invalid trailing UTF-8 octet","time":"2023-04-09T11:05:49+05:30","caller":"/Users/personal/documents/warrant/pkg/config/config.go:88","message":"Error while reading warrant.yaml. Shutting down."}

The caller part is where the entire file path is shown . I tested the mentioned change in config.go

@akajla09
Copy link
Member Author

Can you submit a PR for this change? Looks like it does add the caller to error messages which is nice. However, I'm curious if it'll work for errors that are wrapped via Errors.wrap() (I can check that from your PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants