Refactor : Structured logging in util.go#193
Refactor : Structured logging in util.go#193Joshna907 wants to merge 1 commit intofossology:mainfrom
Conversation
|
@Kaushl2208 You could check this pull request, its done now |
Joshna907
force-pushed
the
utils-new
branch
2 times, most recently
from
1da0c65 to
9f0660b
Compare
pkg/utils/util.go
Outdated
| if status == IMPORT_FAILED { | ||
| shortname := "" | ||
| if result.Shortname != nil { | ||
| shortname = *result.Shortname | ||
| } | ||
|
|
||
| logger.LogError( | ||
| "License import failed", | ||
| zap.String("shortname", shortname), | ||
| zap.String("message", msg), | ||
| ) | ||
| } |
There was a problem hiding this comment.
This results in duplicate logs. Since the error is already logged inside InsertOrUpdateLicenseOnImport, please remove the extra LogError here.
pkg/utils/util.go
Outdated
| shortname := "" | ||
| if result.Shortname != nil { | ||
| shortname = *result.Shortname | ||
| } | ||
|
|
||
| logger.LogError( | ||
| "License validation failed", | ||
| zap.String("shortname", shortname), |
There was a problem hiding this comment.
no need of this , use directly *result.Shortname at logs !
| var licenses []models.LicenseJson | ||
| // Read the content of the data file. | ||
|
|
||
| // Read JSON file | ||
| byteResult, err := os.ReadFile(datafile) | ||
| if err != nil { | ||
| log.Fatalf("Unable to read JSON file: %v", err) | ||
| logger.LogError("Unable to read JSON file", zap.Error(err)) | ||
| return | ||
| } | ||
| // Unmarshal the JSON file data into a slice of LicenseJson structs. | ||
|
|
||
| // Parse JSON | ||
| if err := json.Unmarshal(byteResult, &licenses); err != nil { | ||
| log.Fatalf("error reading from json file: %v", err) | ||
| logger.LogError("Error unmarshalling JSON file", zap.Error(err)) | ||
| return | ||
| } | ||
|
|
||
| // Fetch SUPER_ADMIN user | ||
| user := models.User{} | ||
| level := "SUPER_ADMIN" | ||
|
|
||
| if err := db.DB.Where(&models.User{UserLevel: &level}).First(&user).Error; err != nil { | ||
| log.Fatalf("Failed to find a super admin") | ||
| logger.LogError( | ||
| "Failed to find a super admin", | ||
| zap.String("level", level), | ||
| zap.Error(err), | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
Please replace LogError + return with LogFatal. This code runs during startup, and if it fails the backend should not continue running in a partially initialized state.
|
|
||
| query := fmt.Sprintf("SET pg_trgm.similarity_threshold = %f", threshold) | ||
| if err := db.DB.Exec(query).Error; err != nil { | ||
| log.Println("Failed to set similarity threshold:", err) |
There was a problem hiding this comment.
log the query execution error, with the previous log message
pkg/utils/util.go
Outdated
| // Populatedb populates the database with license data from a JSON file. | ||
| // Populatedb populates the database with license data from a JSON file. |
There was a problem hiding this comment.
Please review and test your changes yourself first, and avoid updating comments unnecessarily.
There was a problem hiding this comment.
Sorry about the messy history! I ran into some bad merge conflicts with main earlier. I've cleaned it all up now, so this PR is just the logging changes in util.go
Please review and test your changes yourself first, and avoid updating comments unnecessarily.
Joshna907
changed the title
Joshna907
force-pushed
the
utils-new
branch
6 times, most recently
from
4590680 to
7560a59
Compare
pkg/utils/util.go
Outdated
| log.Println("Failed to set similarity threshold:", err) | ||
| logger.LogError( | ||
| "Failed to set similarity threshold", | ||
| zap.String("query", query), |
There was a problem hiding this comment.
no need to log the query !
pkg/utils/util.go
Outdated
| logger.LogError( | ||
| "License validation failed", | ||
| zap.String("shortname", *result.Shortname), | ||
| zap.String("error", fmt.Sprintf("field '%s' failed validation: %s", err.(validator.ValidationErrors)[0].Field(), err.(validator.ValidationErrors)[0].Tag())), |
There was a problem hiding this comment.
directly log the field and tag with zap.String , and log the err with zap.Error
|
The rest looks good! |
Thank you ! It's done now and thanks for suggestions |
|
Looks good! |
Kaushl2208
left a comment
Kaushl2208
left a comment
There was a problem hiding this comment.
Changes looks good. But @Joshna907 as much as I like merging and Reviewing PRs. I hope this ZAP Log update ends with 1 PR. It will be nightmare for me to RollBack if something breaks. So if you are expecting any other file with similar changes. I request you to update all that into this PR itself. Be it any file but make all the changes here in this PR. And if we have already updated this, Let me know. I will merge.
@Kaushl2208 I've updated this PR to include the changes from #175 and #176 as requested, as they were already approved but only merging was remaining, so now this PR contains all the remaining structured logging refactors in one place. Should I close this other two PR, Thanks for the review!! |
|
Can we merge ?? @Kaushl2208 |
3 participants
Implemented structured logging in pkg/utils/util.go using zap. A previous pull request was closed during development due to conflicts and mixed changes. This PR is created fresh to keep the scope clean.