Logger #191
Logger #191
Conversation
There was a problem hiding this comment.
You are using 'log.info' only in 'main.go'. Could you include logging across all templates where it is appropriate? I thought that was the initial idea?
Documentation updates are also needed. You can add the new tree structure in 'index.md' and provide a brief explanation in 'creating-project.md'.
Please check why tests are not passing
OK. That's why I wanted to clarify what I was going to do in prev comment because I thought all of them use the same main.go template. I see now that fiber uses a different one, that's it or are there other templates to take into consideration?
I'll add it. I just saw it
I didn't read the CI/CD pipeline so I'm still unaware of what it does. I'll take a look and if I'm stuck I'll ping you. Cheers! PS: Please ping me if i'm missing anything else. Thanks! |
|
@Ujstor Hi again. After an illegal amount of time I think I know why the CI/CD pipeline is angry at me: jobs:
install_dependencies:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Setup Go
uses: actions/setup-go@v4
with:
go-version: "1.20"
- name: Install Dependencies
run: go mod download
framework_matrix:
needs: install_dependencies
strategy:
matrix:
framework:
[chi, gin, fiber, gorilla/mux, httprouter, standard-library, echo]
driver:
[mysql, postgres, sqlite, mongo, redis, none]
goVersion: ["1.20"]slog was experimental in 1.20 and the actual release is in 1.21 That's why I get this fuckery: I think. I tried Also added the template to fiber as well. I just need to create a subdirectory in the internal directory and that's all, I believe. Cheers |
|
I believe I may have misunderstood some things around implementation. I envision an implementation where, after a user creates a 'go-blueprint' project, a custom logger is automatically integrated into all generated template files (such as 'server.go,' 'database.go,' 'routes.go,' etc., depending on the frameworks, databases, or advanced options selected). This approach would provide users with comprehensive examples of how the logger is utilized throughout the entire project. Consequently, users would gain a better understanding of its usage and be able to implement it more effectively in their own projects Check cons in program.go and logic how new dir/subdir is created. In generate-linter.yml, we can change the Go version, and the linter would not encounter any problems. Sorry for delay, I am little busy lately. |
I totally understand. I am as well. I think I have a better understanding of what you want and I think it's a good idea. I'll try to get into it as soon as possible and if I feel like I'm being too opinionated, i'll comment out the code but leave it there in case the user wishes to uncomment it. Do you want me to update the linter myself? I believe updating the framework_matrix task would solve the issue but since it's the CI pipeline, I didn't want to touch it without bringing it up. Cheers |
|
Yeah, you can update the Go version in the linter, and that should do the trick. Your logger is fine, use it in other packages and replace old logs with the new implementation. The current one is not ideal: log.Fatalf(fmt.Sprintf("db down: %v", err))Don't rush with code, there is another big PR that introduces websockets, and there are a lot of changes in templates. We can find the best way to implement it after ws PR is merged. Hit me up on Discord in DM; my username is the same, and we can discuss and find the best solution. |
Hi sorry for the delay. I tried pinging you but you can't be added via strangers.
I will generate some projects this weekend and reread our convo to make sure I put the logger in the right subdir and then use it throughout the projects. If you add me, i'll ping you there before I commit. P.S: I'll update the linter version as well |
|
ci/cd is updated to use 1.22 while linting. it's better to sync the versions, 1.21.1 is used somewhere (that's just a suggestion, do what's best for the proj) logger is put in its own pkg under internal i added a Fatal method to the logger, it called log.Fatal under the hood. slog offers no API for fataling if you want it to be used in the db initialisation for all the templates, we'd either do a dependancy injection on the New() for each db and use the logger therein. but since log.Fatal is used inside, I don't know if it's worth it. but it's doable |
| } | ||
| s := &service{db: db} | ||
| return s | ||
| return s, nil |
There was a problem hiding this comment.
You don't have to set nil for this, because nil is already handled by the caller when you use it somewhere.
There was a problem hiding this comment.
You don't have to set
nilfor this, becausenilis already handled by the caller when you use it somewhere.
check with uj and his discussion with me. we don't want fatal's. this is Go not Java. The idea is to return errors and handle the errors via a tracing mechanism on the top level then terminating on certain errors and ultimately let the user choose what is fatal and what's not instead of greping for all fatals in the project like i did and changing them because that's lackluster in any production environment.
There was a problem hiding this comment.
You don't have to set
nilfor this, becausenilis already handled by the caller when you use it somewhere.check with uj and his discussion with me. we don't want fatal's. this is Go not Java. The idea is to return errors and handle the errors via a tracing mechanism on the top level then terminating on certain errors and ultimately let the user choose what is fatal and what's not instead of greping for all fatals in the project like i did and changing them because that's lackluster in any production environment.
It's not about Go or Java. The concern is about handling errors properly. Just imagine if you don't use fatal, then trying to connect to a database that is down could lead to unexpected behavior or crashes.
There was a problem hiding this comment.
the current implementation is already corrected using fatal and without nil
example:
There was a problem hiding this comment.
this how it work with fatal
$ go run cmd/api/main.go
2024/04/07 16:28:30 [H0llyW00dzZ Firewall] [INFO] Connecting to database: *********
2024/04/07 16:28:31 [H0llyW00dzZ Firewall] [FATAL] Failed to create firewall table: Error 1045 (28000): Access denied for user 'root'@'127.0.0.1' (using password: YES)
exit status 1
There was a problem hiding this comment.
Also when it correct it show like this
$ go run cmd/api/main.go 2024/04/07 20:09:06 [H0llyW00dzZ Firewall] [INFO] Connecting to database: ********* 2024/04/07 20:09:08 [H0llyW00dzZ Firewall] [INFO] Successfully initialized the firewall table. 2024/04/07 20:09:09 [H0llyW00dzZ Project] [INFO] Starting server on :8080 ┌───────────────────────────────────────────────────┐ │ H0llyW00dzZ Project │ │ Fiber v2.52.4 │ │ http://127.0.0.1:8080 │ │ (bound on host 0.0.0.0 and port 8080) │ │ │ │ Handlers ............ 539 Processes ........... 1 │ │ Prefork ....... Disabled PID ............. 28260 │ └───────────────────────────────────────────────────┘
Hey - I thought I should chime in here because this great PR has definitely been out for too long without resolution. I discussed with the internal team, and we believe for go-blueprint that thing that makes sense is to throw Fatal if there is a DB connectivity error.
I am thinking about this from the perspective of a user, and if there is a an error connecting to the DB, but the app still spins up, it might be less than perfect user experience?
My intention is to not come in here and throw down a hammer, but again, asking for input on this. From my personal thinking, fatal on db error makes sense
There was a problem hiding this comment.
Also when it correct it show like this
$ go run cmd/api/main.go 2024/04/07 20:09:06 [H0llyW00dzZ Firewall] [INFO] Connecting to database: ********* 2024/04/07 20:09:08 [H0llyW00dzZ Firewall] [INFO] Successfully initialized the firewall table. 2024/04/07 20:09:09 [H0llyW00dzZ Project] [INFO] Starting server on :8080 ┌───────────────────────────────────────────────────┐ │ H0llyW00dzZ Project │ │ Fiber v2.52.4 │ │ http://127.0.0.1:8080 │ │ (bound on host 0.0.0.0 and port 8080) │ │ │ │ Handlers ............ 539 Processes ........... 1 │ │ Prefork ....... Disabled PID ............. 28260 │ └───────────────────────────────────────────────────┘Hey - I thought I should chime in here because this great PR has definitely been out for too long without resolution. I discussed with the internal team, and we believe for go-blueprint that thing that makes sense is to throw Fatal if there is a DB connectivity error.
I am thinking about this from the perspective of a user, and if there is a an error connecting to the DB, but the app still spins up, it might be less than perfect user experience?
My intention is to not come in here and throw down a hammer, but again, asking for input on this. From my personal thinking, fatal on db error makes sense
I am thinking about this from the perspective of a user, and if there is a an error connecting to the DB, but the app still spins up, it might be less than perfect user experience? For that, it's better to use Fatal for the database because, for the database, it's recommended to set it in init to initialize first before the app runs. So, when the database fails, the app won't run.
I have already implemented this, as shown in the example below:
func init() {
// Initialize the logger here with the necessary parameters.
Logger.InitializeLogger("H0llyW00dzZ Firewall")
// Initialize the context and cancel function
schedulerCtx, cancelScheduler = context.WithCancel(context.Background())
// Check if the necessary environment variables for the database connection are set.
if os.Getenv(database.EnvMYSQLDBName) != "" && os.Getenv(database.EnvMYSQLDBPassword) != "" &&
os.Getenv(database.EnvMYSQLDBUsername) != "" && os.Getenv(database.EnvMYSQLDBPort) != "" &&
os.Getenv(database.EnvMYSQLDBHost) != "" {
// Initialize the database service.
db = database.New()
// Initialize the firewall table if it doesn't exist.
if err := createFirewallTable(); err != nil {
Logger.LogFatalf("Failed to initialize firewall table: %v", err)
} else {
Logger.LogInfo("Successfully initialized the firewall table.")
}
}
}also note that keeping fatal in New in this
func New() Service {
// Opening a driver typically will not attempt to connect to the database.
db, err := sql.Open("mysql", fmt.Sprintf("%s:%s@tcp(%s:%s)/%s", username, password, host, port, dbname))
if err != nil {
// This will not be a connection error, but a DSN parse error or
// another initialization error.
log.Fatal(err)
}it will show the failure in // Initialize the firewall table if it doesn't exist. plus it won't making new connection during Initialize because of this singleton pattern:
There was a problem hiding this comment.
Also, ignore the line schedulerCtx, cancelScheduler = context.WithCancel(context.Background()) in example. That is for my worker goroutines.
There was a problem hiding this comment.
Another Example Method Using Recovery for Panic
func init() {
// Initialize the logger here with the necessary parameters.
Logger.InitializeLogger("H0llyW00dzZ Firewall")
// Initialize the context and cancel function
schedulerCtx, cancelScheduler = context.WithCancel(context.Background())
// Check if the necessary environment variables for the database connection are set.
if os.Getenv(database.EnvMYSQLDBName) != "" && os.Getenv(database.EnvMYSQLDBPassword) != "" &&
os.Getenv(database.EnvMYSQLDBUsername) != "" && os.Getenv(database.EnvMYSQLDBPort) != "" &&
os.Getenv(database.EnvMYSQLDBHost) != "" {
// Set up a defer to recover from any panic that might occur in database initialization.
defer func() {
if r := recover(); r != nil {
Logger.LogFatalf("Failed to initialize the database: %v", r)
}
}()
// Initialize the database service.
db = database.New()
// Initialize the firewall table if it doesn't exist.
if err := createFirewallTable(); err != nil {
Logger.LogFatalf("Failed to initialize firewall table: %v", err)
} else {
Logger.LogInfo("Successfully initialized the firewall table.")
}
}
}It will show like this:
$ go run cmd/api/main.go
2024/04/10 08:36:54 [H0llyW00dzZ Firewall] [INFO] Initialization to database: *********
2024/04/10 08:36:55 [H0llyW00dzZ Firewall] [FATAL] Failed to initialize the database: Error 1045 (28000): Access denied for user 'root'@'127.0.0.1' (using password: YES)
exit status 1There was a problem hiding this comment.
And another issue that can occur during runtime (e.g., when a user is interacting with the database but the database goes down) can be covered by panic recovery in the middleware (e.g., if the error is not handled correctly).
That's it
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) | ||
| defer cancel() | ||
|
|
||
| err := s.db.PingContext(ctx) | ||
| if err != nil { | ||
| log.Fatalf(fmt.Sprintf("db down: %v", err)) | ||
| return nil, fmt.Errorf("error pinging db: %w", err) |
There was a problem hiding this comment.
For this one as well, it's important to use fatal.
| } | ||
|
|
||
| return map[string]string{ | ||
| "message": "It's healthy", | ||
| } | ||
| } | ||
| }, nil |
There was a problem hiding this comment.
For this one as well, you don't have to set nil.
|
|
||
| return map[string]string{ | ||
| "message": "It's healthy", | ||
| } | ||
| } | ||
| }, nil |
There was a problem hiding this comment.
This approach avoids explicit nil values for Redis and MySQL connections, which is beneficial for large codebases and scalability. It's better to remove explicit nil handling, as it will be handled by the caller elsewhere.
| source.File = filepath.Base(source.File) | ||
| } | ||
|
|
||
| // if used in AWS or GCP (and Azure?), they'll add the timestamp themselves. Else uncomment to get timestamps |
There was a problem hiding this comment.
It's better to enable this independently, even when used in AWS, GCP, or Azure, because the logger should be configured independently instead of depending on the cloud provider or whatever it is for timestamp handling.
|
I am closing this repo, assuming there is no interest to continue implementation. |
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
Problem/Feature
Issue #182
Description of Changes:
This PR introduces a logger template and updates the main template to include the logger. Here are the detailed changes:
Logger Template Added: A new template for the logger has been added at:
cmd/template/framework/files/main/logger.go.tmplMain Template Updated: The main template file has been updated to incorporate the logger.
cmd/template/framework/files/main/main.go.tmplprogram.go Updated: The program.go file has been updated to inject the logger.go file in the internal path.
cmd/program/program.gomain.go Updated: The main.go file has been updated to embed the logger template.
cmd/template/framework/main.goChecklist