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

feat(log): add Logger() method for Helper #3443

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Conversation

DCjanus
Copy link
Contributor

@DCjanus DCjanus commented Oct 21, 2024

Description

In the real world, some log fields have significant contextual relevance. A reasonable approach is to use log.With to attach fields and create a new Logger, then convert it to *log.Helper held by an object.

For statically created Loggers, this works well; but for dynamically created Loggers, such as when an API request contains a user ID and we perform many operations for it, in order to easily query logs afterwards, we need to include his user ID at each logging point.

Because Service objects generally only contain *log.Helper, there is currently no public method to obtain log.Logger from *log.Helper and add fields, and we have to pass all those fields to every logging point, which is quiet boring.

Show case

package kratos

import (
	"fmt"
	"github.com/go-kratos/kratos/v2/middleware/tracing"
	"os"

	"github.com/go-kratos/kratos/v2/log"
)

type Worker struct {
	log *log.Helper
}

func main() {
	id, _ := os.Hostname()
	logger := log.With(log.NewStdLogger(os.Stdout),
		"ts", log.DefaultTimestamp,
		"caller", log.DefaultCaller,
		"service.id", id,
		"service.name", Name,
		"service.version", Version,
		"trace.id", tracing.TraceID(),
		"span.id", tracing.SpanID(),
	)
	var workerId string
	// some logic to get workerId
	NewWorker(logger, workerId).Work()
}

func NewWorker(logger log.Logger, workerId string) *Worker {
	logger = log.With(logger, "module", "worker", "worker_id", workerId)
	return &Worker{
		log: log.NewHelper(logger),
	}
}

// Work method is an example of how to use the new method `(*log.Helper).Logger()` to get the original logger and
// attach some fields to it. This is useful when you want to identify some concurrent goroutines.
func (w *Worker) Work() {
	// ... some preparation work that may emit logs

	// in the real world, tasks are created dynamically, and the number of tasks is not fixed
	for id := 0; id < 10; id++ {
		workerId := fmt.Sprintf("sub_worker_%d", id)
		if id%2 == 0 {
			go NewSubWorker(w.log.Logger(), workerId).Work()
		} else {
			go NewSubWorkerWithoutSubLogger(w.log, workerId).Work()
		}

	}
}

type SubWorker1 struct {
	log *log.Helper
}

func NewSubWorker(logger log.Logger, subWorkerId string) *SubWorker1 {
	logger = log.With(logger, "module", "sub_worker", "sub_worker_id", subWorkerId)
	return &SubWorker1{
		log: log.NewHelper(logger),
	}
}

func (w *SubWorker1) Work() {
	var err error
	// do something
	if err != nil {
		w.log.Errorw(
			"event", "error_event_1",
			"error", err,
		)
	}
	// do something
	if err != nil {
		w.log.Errorw("event", "error_event_2", "error", err)
	}

	// and so on, you can add more log points here

	return
}

type SubWorkerWithoutSubLogger struct {
	log  *log.Helper
	name string
}

func NewSubWorkerWithoutSubLogger(l *log.Helper, name string) *SubWorkerWithoutSubLogger {
	return &SubWorkerWithoutSubLogger{
		log:  l,
		name: name,
	}
}

func (w *SubWorkerWithoutSubLogger) Work() {
	var err error
	// do something
	if err != nil {
		// without the new method, you have to attach the fields every time
		w.log.Errorw(
			"module", "sub_worker",
			"sub_worker_id", w.name,
			"event", "error_event_1",
			"error", err,
		)
	}
	// do something
	if err != nil {
		w.log.Errorw(
			"module", "sub_worker",
			"sub_worker_id", w.name,
			"event", "error_event_2",
			"error", err,
		)
	}

	// and so on, you can add more log points here

	return
}

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Oct 21, 2024
@DCjanus DCjanus changed the title feat(log): add Logger() method for Helper Draft: feat(log): add Logger() method for Helper Oct 21, 2024
@DCjanus DCjanus marked this pull request as draft October 21, 2024 13:09
@DCjanus DCjanus changed the title Draft: feat(log): add Logger() method for Helper feat(log): add Logger() method for Helper Oct 21, 2024
@DCjanus DCjanus marked this pull request as ready for review October 21, 2024 13:09
@DCjanus DCjanus marked this pull request as draft October 21, 2024 13:09
@DCjanus
Copy link
Contributor Author

DCjanus commented Oct 21, 2024

@shenqidebaozi feel free to comment something if you want

@shenqidebaozi shenqidebaozi marked this pull request as ready for review October 28, 2024 06:39
@DCjanus
Copy link
Contributor Author

DCjanus commented Oct 28, 2024

I have communicated with @shenqidebaozi about this PR through other channels, and I'd like to sync the discussion content here:

@shenqidebaozi believes that With method should enough to be exposed to LogHelper.

But I think the With method is not a lightweight operation (it requires copying an entire list), and providing an easily callable With method might lead users to misuse it; exposing only the Logger method can encourage users to reuse Logger instance. And @shenqidebaozi agreement with that.

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Oct 28, 2024
@DCjanus
Copy link
Contributor Author

DCjanus commented Nov 4, 2024

@shenqidebaozi Hi, would you mind to take a look on this PR, maybe it's time to merge this

@demoManito demoManito merged commit f8b97f6 into go-kratos:main Nov 5, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants