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

fix(servicegroup): use logx for shutdown message #3719

Merged
merged 1 commit into from
Dec 16, 2023

Conversation

AlexLast
Copy link
Contributor

Currently when stopping a service in a ServiceGroup the standard logger is used, this changes the log line to use the logx package and ensure all log lines are of the same format.

@kevwan
Copy link
Contributor

kevwan commented Nov 22, 2023

Here, we cannot guarantee that logx is still available, it might be closed already.

@kevwan kevwan added the kind/need-more-discussion Not decided, need more discussion! label Nov 22, 2023
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Merging #3719 (8017c1f) into master (4003864) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files
Files Coverage Δ
core/service/servicegroup.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@AlexLast
Copy link
Contributor Author

Here, we cannot guarantee that logx is still available, it might be closed already.

@kevwan Thanks for the feedback, what's the expected behaviour when logx is closed? With the default logx.LogConf I don't encounter any issues with this code. Does this only cause issues when writing to file?

I also can't seem to force an issue by manually closing logx, for example, the below still outputs logs:

logx.Close()
logx.Info("Foo")

Ideally I'd like my service logs to all be the same format (json) to keep parsing/shipping easier. Let me know what you think?

@kevwan
Copy link
Contributor

kevwan commented Dec 16, 2023

Here, we cannot guarantee that logx is still available, it might be closed already.

@kevwan Thanks for the feedback, what's the expected behaviour when logx is closed? With the default logx.LogConf I don't encounter any issues with this code. Does this only cause issues when writing to file?

I also can't seem to force an issue by manually closing logx, for example, the below still outputs logs:

logx.Close()
logx.Info("Foo")

Ideally I'd like my service logs to all be the same format (json) to keep parsing/shipping easier. Let me know what you think?

If logx is closed already, logs are printed to console.

You can try it with the following code.

package main

import (
	"time"

	"github.com/zeromicro/go-zero/core/conf"
	"github.com/zeromicro/go-zero/core/logx"
)

func main() {
	var c logx.LogConf
	if err := conf.FillDefault(&c); err != nil {
		panic(err)
	}
	c.Mode = "file"
	logx.MustSetup(c)
	logx.Info("hello")
	logx.Close()

	time.Sleep(time.Second)
	logx.Info("world")
}

Copy link
Contributor

@kevwan kevwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@kevwan kevwan added this pull request to the merge queue Dec 16, 2023
Merged via the queue into zeromicro:master with commit 919477f Dec 16, 2023
6 checks passed
WqyJh pushed a commit to WqyJh/go-zero that referenced this pull request Dec 21, 2023
dongmeng199 pushed a commit to dongmeng199/go-zero that referenced this pull request Dec 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/need-more-discussion Not decided, need more discussion!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants