Improve logging code #7

Merged
fnetX merged 7 commits from :add-logging into main 3 months ago
Gusted commented 3 months ago
Collaborator
  • Currently the logging isn't that great, it uses fmt.Printf("xxx\n") where a error is being logged with no prefix or any idea which function called it.
  • This PR adds a logging module that is able to log to multiple sources, will display the date + time + filename + caller/linenumber of the function that called the log.XXX(), this should make the log file on the server-side more fun to read and easier to understand why something has errored.
  • I did touch some other lines not related to logging to make it more idiomatic.
- Currently the logging isn't that great, it uses fmt.Printf("xxx\n") where a error is being logged with no prefix or any idea which function called it. - This PR adds a logging module that is able to log to multiple sources, will display the date + time + filename + caller/linenumber of the function that called the log.XXX(), this should make the log file on the server-side more fun to read and easier to understand why something has errored. - I did touch some other lines not related to logging to make it more idiomatic.
Gusted added 3 commits 3 months ago
073491f30f
Add logger module
426d2af661
Add logging section to config
Collaborator

Thank you. Your PR doesn't include the more recent commit 04c5f3c40e that already heavily touched error handling. Please use this as a base, because it shifts error handling + logging to one place.

If there are left over print statements, they should be updated to use this approach if possible.

Thank you. Your PR doesn't include the more recent commit https://codeberg.org/Codeberg/moderation/commit/04c5f3c40edb3fe0961986a29ddd20ee5a162e2b that already heavily touched error handling. Please use this as a base, because it shifts error handling + logging to one place. If there are left over print statements, they should be updated to use this approach if possible.
Poster
Collaborator

Thank you. Your PR doesn't include the more recent commit 04c5f3c40e that already heavily touched error handling. Please use this as a base, because it shifts error handling + logging to one place.

If there are left over print statements, they should be updated to use this approach if possible.

Yeah I just noticed... I'm not sure if the creative approach is idiomatic or good. But I will try to use it as base for now.

> Thank you. Your PR doesn't include the more recent commit https://codeberg.org/Codeberg/moderation/commit/04c5f3c40edb3fe0961986a29ddd20ee5a162e2b that already heavily touched error handling. Please use this as a base, because it shifts error handling + logging to one place. > > If there are left over print statements, they should be updated to use this approach if possible. Yeah I just noticed... I'm not sure if the creative approach is idiomatic or good. But I will try to use it as base for now.
Gusted added 1 commit 3 months ago
Poster
Collaborator

I've tried to be in-line of that commit. I had to adjust it a bit in order to have have the call hierachy correct when log.XXX will be called so the correct function + line/filename can be shown in the log.

The commit also a includes for the shuffling of the slice(doing a random int of the slice has a good chance that the random int will land onto something that already has been "searched" for).

I've tried to be in-line of that commit. I had to adjust it a bit in order to have have the call hierachy correct when log.XXX will be called so the correct function + line/filename can be shown in the log. The commit also a includes for the shuffling of the slice(doing a random int of the slice has a good chance that the random int will land onto something that already has been "searched" for).
Gusted added 1 commit 3 months ago
02329deda6
Don't require `fmt.Errorf`
Gusted added 1 commit 3 months ago
3188c968c2
Be more natural in error handling
fnetX reviewed 3 months ago
}
func ErrorMiddleware(fn func(w http.ResponseWriter, r *http.Request) error) func(w http.ResponseWriter, r *http.Request) {
return func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "text/json")
fnetX commented 3 months ago
Collaborator

https://codeberg.org/Codeberg/moderation/src/branch/main/backend/main.go#L117 I think this is unnecessary as of this line. Sorry, I missed this when I added it myself.

https://codeberg.org/Codeberg/moderation/src/branch/main/backend/main.go#L117 I think this is unnecessary as of this line. Sorry, I missed this when I added it myself.
Gusted marked this conversation as resolved
backend/main.go Outdated
if err != nil {
fmt.Printf("Failed to read file: %v\n", err)
os.Exit(1)
log.Fatal("ini.Load[file=%q]: %v", "moderation_backend.ini", err)
fnetX commented 3 months ago
Collaborator

what's filled into the %q? Where does it take the value from?

what's filled into the %q? Where does it take the value from?
Poster
Collaborator

moderation_backend.ini is the value and %q means print as quoted with ". But given this is hard-coded it should be removed.

`moderation_backend.ini` is the value and `%q` means print as quoted with `"`. But given this is hard-coded it should be removed.
fnetX commented 3 months ago
Collaborator

ah oh, sorry, I overlooked the second parameter. I thought it was one long string and err and wondered where the second value should be coming from :)

ah oh, sorry, I overlooked the second parameter. I thought it was one long string and err and wondered where the second value should be coming from :)
Gusted marked this conversation as resolved
backend/main.go Outdated
mailer.MailConfig(cfg)
userConfig(cfg)
repoConfig(cfg)
log.LogConfig(cfg)
fnetX commented 3 months ago
Collaborator

by the way, you proposed to me to rename this just to blah.Config(). I somehow thought I already changed it, but looks like I didn't yet. Feel free to update this if you'd like.

by the way, you proposed to me to rename this just to blah.Config(). I somehow thought I already changed it, but looks like I didn't yet. Feel free to update this if you'd like.
Gusted marked this conversation as resolved
}
_, res, err := giteaClient.TransferRepo(currentOwner, currentName, gitea.TransferRepoOption{
_, _, err := giteaClient.TransferRepo(currentOwner, currentName, gitea.TransferRepoOption{
fnetX commented 3 months ago
Collaborator

is the result status code included in the error?

is the result status code included in the error?
Poster
Collaborator

No. But I personally don't see the benefit of the status code when you can instead print the error. Because go-sdk should convert the status code(best-effort) to a more generalized error.

No. But I _personally_ don't see the benefit of the status code when you can instead print the error. Because go-sdk should convert the status code(best-effort) to a more generalized error.
Poster
Collaborator
Ref: https://gitea.com/gitea/go-sdk/src/commit/a56a62a4df29fde9a34ea2a09e9c351d3ea96699/gitea/client.go#L303
fnetX commented 3 months ago
Collaborator

I had an issue recently that I run code on already transferred repos, and the status code was 405 method not allowed. It didn't really help me with debugging, but the SDK message wasn't more helpful I think. I just hope that the improved error handling would have avoided this issue in the 1st place, so I don't object to leaving this as-is.

I had an issue recently that I run code on already transferred repos, and the status code was 405 method not allowed. It didn't really help me with debugging, but the SDK message wasn't more helpful I think. I just hope that the improved error handling would have avoided this issue in the 1st place, so I don't object to leaving this as-is.
fnetX marked this conversation as resolved
Gusted added 1 commit 3 months ago
ee3968a077
Apply code suggestions
fnetX merged commit 2b47c34a12 into main 3 months ago
fnetX referenced this issue from a commit 3 months ago
Gusted deleted branch add-logging 3 months ago
ci/woodpecker/pr/backend Pipeline was successful
The pull request has been merged as 2b47c34a12.
Sign in to join this conversation.
Loading…
There is no content yet.