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

[Feature Request] Logging support #182

Open
1 task done
spa5k opened this issue Feb 8, 2024 · 24 comments
Open
1 task done

[Feature Request] Logging support #182

spa5k opened this issue Feb 8, 2024 · 24 comments
Labels
enhancement New feature or request

Comments

@spa5k
Copy link

spa5k commented Feb 8, 2024

Tell us about your feature request

Something like global setup of loggers, zap/slog/zerolog etc, and optimal way of importing the logger in various places

Disclaimer

  • I agree
@spa5k spa5k added the enhancement New feature or request label Feb 8, 2024
@Ujstor
Copy link
Collaborator

Ujstor commented Feb 13, 2024

We believe that incorporating Slog or the standard logging library from Go would be the most suitable approach.

@mcheviron
Copy link

We believe that incorporating Slog or the standard logging library from Go would be the most suitable approach.

Will you add it or should someone make a pull request?
Also for whoever does it, I suggest creating an abstraction over LogAttrs instead of using the slog API. Otherwise each log will alloc on the heap, unlike Zap that uses this strategy by default

@Ujstor
Copy link
Collaborator

Ujstor commented Feb 14, 2024

This is open for anyone willing to step forward and address this enhancement. Just let me know, and I can assign it

@briancbarrow
Copy link
Collaborator

Also for whoever does it, I suggest creating an abstraction over LogAttrs instead of using the slog API. Otherwise each log will alloc on the heap, unlike Zap that uses this strategy by default

@mcheviron Do you have any suggested reading about this before someone takes this on?

@mcheviron
Copy link

Also for whoever does it, I suggest creating an abstraction over LogAttrs instead of using the slog API. Otherwise each log will alloc on the heap, unlike Zap that uses this strategy by default

@mcheviron Do you have any suggested reading about this before someone takes this on?

https://pkg.go.dev/log/[email protected]#LogAttrs

The interface would be simple. A simple logger struct that exposes the same API but has a private context inside of it to enable the LogAttrs method. Then this would allow the caller to just send messages to the API for .Info, .Debug etc and the user won't feel a difference but the allocations will remain in the stack. This is how Zap does it internally. The standard slog API uses any which is a heap allocation. You can go run -gcflags "-m" and observe it

As for implementing it. How would you like implementing it? Should this logger be put in a separate file in the cmd dir or how would it work for your project? I have the code. I created it for work but i'd like to know how best is it to contribute it to your project

P.S: https://www.reddit.com/r/golang/comments/19882h2/slog_why_do_my_attrs_escape_to_the_heap/?utm_source=share&utm_medium=web2x&context=3
Not quality content but we discussed it there before

@spa5k
Copy link
Author

spa5k commented Feb 14, 2024

Rather than this, what if we use slog, then implement its handlers in various loggers, like zap etc.

package main

import (
    "log/slog"

    "go.uber.org/zap"
    "go.uber.org/zap/exp/zapslog"
)

func main() {
    zapL := zap.Must(zap.NewProduction())

    defer zapL.Sync()

    logger := slog.New(zapslog.NewHandler(zapL.Core(), nil))

    logger.Info(
        "incoming request",
        slog.String("method", "GET"),
        slog.String("path", "/api/user"),
        slog.Int("status", 200),
    )
}

@mcheviron
Copy link

I don't see the value of adding Zap since the original commenter wanted to use slog to keep things in the stdlib. If all you need is to pass the Zap core to slog then why not just stick with zap? That sounds like over engineering something, at least for me.

If they agree, we can handle logging like web frameworks, expose the stdlib only option as the suggested but offer Zap and friends as options.

@Ujstor
Copy link
Collaborator

Ujstor commented Feb 16, 2024

@mcheviron, if you agree, I would like to assign this to you. The solution needs to be simple and without overengineering, working out of the box and being incorporated into the blueprint (users do not have a choice in the CLI).

You can put logging.go in internal/utils. I am not familiar with common practices and we are open for any suggestions.

Maybe you can create a test repo from go-blueprint to see how everything works together.

You already have the code, you know what you're doing.

@mcheviron
Copy link

@mcheviron, if you agree, I would like to assign this to you. The solution needs to be simple and without overengineering, working out of the box and being incorporated into the blueprint (users do not have a choice in the CLI).

You can put logging.go in internal/utils. I am not familiar with common practices and we are open for any suggestions.

Maybe you can create a test repo from go-blueprint to see how everything works together.

You already have the code, you know what you're doing.

Hi thanks. Please assign it to me and I shall get to it in the weekend. Is there a deadline?
internal/utils is a sensible approach since it's supposed to be internal and a util

I'll do a fork and I'll test in my environment before I pul ofc

@Ujstor
Copy link
Collaborator

Ujstor commented Feb 16, 2024

I was thinking that you implement it first in a project that is created with a blueprint, then we can finalize the structure and functionality. After that, you can simply add changes to existing templates and new logic for creating the utils folder and logging.go. That was just an idea, but you can implement it directly in the project.

There is no deadline, take your time.

@mcheviron
Copy link

I was thinking that you implement it first in a project that is created with a blueprint, then we can finalize the structure and functionality. After that, you can simply add changes to existing templates and new logic for creating the utils folder and logging.go. That was just an idea, but you can implement it directly in the project.

https://github.com/mcheviron/logger-test

Take a look and tell me if this is what you wanted. I tweaked my original implementation by creating a config that the user can use to customise the output (json or text for example). I also added some helpful defaults to smoothen the rough edges of slog. For example making the logger log only the base file if the user asked to add source instead of the absolute path. I also added how they can automatically take away the timestamp in case they run on a cloud. AWS CloudWatch and the respective GCP and Azure options inject timestamps by default. This one is commented, they can uncomment it if they need it.

@Ujstor
Copy link
Collaborator

Ujstor commented Feb 17, 2024

This looks nice, exactly what I was thinking about. What do you think about adding a middleware method? Chi and Echo are using built-in middleware. Can we unify that and use it for all frameworks?

@mcheviron
Copy link

This looks nice, exactly what I was thinking about. What do you think about adding a middleware method? Chi and Echo are using built-in middleware. Can we unify that and use it for all frameworks?

we can but i don't suggest it. I don't know about chi (never used it) but echo has a way to inject your favourite logger into the framework's logger if you want. so my advice would be making the logger's template present for all users no matter their favourite framework and if they want to plug it, they do it. because echo already has a logging middleware that can be used for logging requests, but the logger i created is for tracing purposes mainly.

if a user chose to use a framework, we should get least into their way and let them figure their stuff out, since they already should be familiar with its ecosystem.

what i suggest is just provide the logger. afterwards i was going to make a pull request anyway to provide minimal code (less than 100 LOC) that kickstarts the 1.22 router and make it easy to configure. if you go on my repos i have a repo called sdk that has that code (some of it at least). i use it for work as a minimal http stdlib only sdk that makes injecting middleware, logging etc easy without third party stuff. but that's for later.

if you're okay with that, just logger. i'll look into the template stuff in the create command and figure out how to inject it in a fork and then when it's okay i'll do a pull. cheers

@Ujstor
Copy link
Collaborator

Ujstor commented Feb 17, 2024

I agree, the best way is not to overcomplicate things. When you are ready, open a PR, and I will review it.

@Ujstor
Copy link
Collaborator

Ujstor commented Feb 17, 2024

@mcheviron We have PR #187. I think the best way is to wait until it is merged because it will introduce changes that affect you. Afterward, it would also be simple to implement the logger

@Ujstor
Copy link
Collaborator

Ujstor commented Feb 20, 2024

@mcheviron PR has been merged, you can open yours when you are ready.

@mcheviron
Copy link

@Ujstor thank you. i'll get to it in the weekend

@mcheviron
Copy link

mcheviron commented Feb 23, 2024

@Ujstor

I was thinking of putting the template file here:

  • cmd/template/framework/files/

Then, I'll add the Go embedding here and expose a LoggerTemplate:

  • cmd/template/framework/main.go

Next, I'll add a case in the CreateFileWithInjection function and call it somewhere in the CreateMainFile function.

After this block of code:

	err = p.CreateFileWithInjection(internalServerPath, projectPath, "server.go", "server")
	if err != nil {
		log.Printf("Error injecting server.go file: %v", err)
		cobra.CheckErr(err)
		return err
	}

(i'm adding these comments as confirmation and for me to not forget because i'm a dummy)

Anyways, if ok, cool it'll be done in a couple of minute, not? tell me what you want different. thanks!

@Ujstor
Copy link
Collaborator

Ujstor commented Feb 23, 2024

@mcheviron Looks good to me.

@mcheviron mcheviron mentioned this issue Feb 23, 2024
@Ujstor
Copy link
Collaborator

Ujstor commented Mar 2, 2024

@mcheviron Join Discord you can find me there
https://discord.gg/Kx8Zz46aQS

@H0llyW00dzZ
Copy link
Contributor

I have implemented it a custom logger for logging support.

Example of how it works:

With ENV UNCENSORED set to False (for Production/Server Less)

$ go run cmd/api/main.go
2024/03/31 14:44:15 [H0llyW00dzZ] [INFO] Starting server on :****

 ┌───────────────────────────────────────────────────┐
 │                    H0llyW00dzZ                    │
 │                   Fiber v2.52.4                   │
 │               http://127.0.0.1:8080               │
 │       (bound on host 0.0.0.0 and port 8080)       │
 │                                                   │
 │ Handlers ............ 534 Processes ........... 1 │
 │ Prefork ....... Disabled  PID .............. 6860 │
 └───────────────────────────────────────────────────┘

2024/03/31 14:44:22 [H0llyW00dzZ] [INFO] Shutting down server... reason: interrupt
2024/03/31 14:44:23 [H0llyW00dzZ] [INFO] Disconnected from database: *********
2024/03/31 14:44:23 [H0llyW00dzZ] [INFO] Database connection closed.
2024/03/31 14:44:27 [H0llyW00dzZ] [INFO] Server exiting

Note

With ENV UNCENSORED set to False (for Production/Server Less), it will CENSOR any object formatting object in Golang (e.g., %v, %s, etc).

With ENV UNCENSORED set to True

$ go run cmd/api/main.go
2024/03/31 14:44:15 [H0llyW00dzZ] [INFO] Starting server on :8080

 ┌───────────────────────────────────────────────────┐
 │                    H0llyW00dzZ                    │
 │                   Fiber v2.52.4                   │
 │               http://127.0.0.1:8080               │
 │       (bound on host 0.0.0.0 and port 8080)       │
 │                                                   │
 │ Handlers ............ 534 Processes ........... 1 │
 │ Prefork ....... Disabled  PID .............. 6860 │
 └───────────────────────────────────────────────────┘

2024/03/31 14:44:22 [H0llyW00dzZ] [INFO] Shutting down server... reason: interrupt
2024/03/31 14:44:23 [H0llyW00dzZ] [INFO] Disconnected from database: defaultdb
2024/03/31 14:44:23 [H0llyW00dzZ] [INFO] Database connection closed.
2024/03/31 14:44:27 [H0llyW00dzZ] [INFO] Server exiting

@H0llyW00dzZ
Copy link
Contributor

Also note that the custom logger I have implemented is legit made from scratch and is currently only compatible with the standard library logger.

@Ujstor
Copy link
Collaborator

Ujstor commented Apr 2, 2024

This is a work in progress, and it would take care of proper logging and error handling. Here is draft https://github.com/mcheviron/test

@H0llyW00dzZ
Copy link
Contributor

This is a work in progress, and it would take care of proper logging and error handling. Here is draft https://github.com/mcheviron/test

By the way, it's easy to implement logging and error handling for the Fiber framework, especially for custom loggers.

Example:

// LogUserActivity logs a user activity message along with the client IP and User-Agent.
func LogUserActivity(c *fiber.Ctx, activity string) {
	clientIP := c.IP()
	userAgent := c.Get("User-Agent")
	// Note: `LogVisitorf` is the handler for the example logger using the standard library
	LogVisitorf("Activity: %s - IP: %s, User-Agent: %s", activity, clientIP, userAgent)
}

Then, call it in the handler:

// Log the user activity
Logger.LogUserActivity(c, "viewed firewall IPs")

The output will look like this:

2024/04/04 16:19:07 [H0llyW00dzZ Project] [VISITOR] Activity: viewed firewall IPs - IP: 127.0.0.1, User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0.1

The provided code snippet demonstrates a simple way to implement logging for user activities in the Fiber framework. It includes a LogUserActivity function that logs the user's activity, IP address, and User-Agent string using a custom logger (LogVisitorf). This function can be called within the handler to log relevant user activities.

The example output shows how the logged information would appear, including the timestamp, a custom prefix ([H0llyW00dzZ Project] [VISITOR]), and the activity details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants