Copilot commented on code in PR #765:
URL: https://github.com/apache/dubbo-go-pixiu/pull/765#discussion_r2394864225
##########
pkg/logger/logger_test.go:
##########
@@ -18,66 +18,198 @@
package logger
import (
- "fmt"
+ "os"
"path/filepath"
- "runtime"
+ "strings"
"testing"
+ "time"
)
import (
- "github.com/stretchr/testify/assert"
+ "go.uber.org/zap"
+ "go.uber.org/zap/zapcore"
)
-func TestInitLog(t *testing.T) {
- var (
- err error
- path string
+func newDevConfigToFile(t *testing.T) (*zap.Config, string) {
+ t.Helper()
+ dir := t.TempDir()
+ out := filepath.Join(dir, "zap.log")
+
+ cfg := zap.NewDevelopmentConfig()
+ // output to a temp file
+ cfg.OutputPaths = []string{out}
+ cfg.ErrorOutputPaths = []string{out}
+ cfg.EncoderConfig.StacktraceKey = "stacktrace"
+ cfg.EncoderConfig.CallerKey = "caller"
+
+ return &cfg, out
+}
+
+func readAll(t *testing.T, path string) string {
+ t.Helper()
+ b, err := os.ReadFile(path)
+ if err != nil {
+ t.Fatalf("read %s failed: %v", path, err)
+ }
+ return string(b)
+}
+
+func TestDisableStacktraceTrueNoStackEvenOnError(t *testing.T) {
+ cfg, out := newDevConfigToFile(t)
+ cfg.DisableStacktrace = true // YAML equals to：disableStacktrace: true
Review Comment:
Mixed language comment contains Chinese characters. Should be English-only:
'YAML equivalent: disableStacktrace: true'
```suggestion
cfg.DisableStacktrace = true // YAML equivalent: disableStacktrace: true
```
##########
pkg/logger/logger_test.go:
##########
@@ -18,66 +18,198 @@
package logger
import (
- "fmt"
+ "os"
"path/filepath"
- "runtime"
+ "strings"
"testing"
+ "time"
)
import (
- "github.com/stretchr/testify/assert"
+ "go.uber.org/zap"
+ "go.uber.org/zap/zapcore"
)
-func TestInitLog(t *testing.T) {
- var (
- err error
- path string
+func newDevConfigToFile(t *testing.T) (*zap.Config, string) {
+ t.Helper()
+ dir := t.TempDir()
+ out := filepath.Join(dir, "zap.log")
+
+ cfg := zap.NewDevelopmentConfig()
+ // output to a temp file
+ cfg.OutputPaths = []string{out}
+ cfg.ErrorOutputPaths = []string{out}
+ cfg.EncoderConfig.StacktraceKey = "stacktrace"
+ cfg.EncoderConfig.CallerKey = "caller"
+
+ return &cfg, out
+}
+
+func readAll(t *testing.T, path string) string {
+ t.Helper()
+ b, err := os.ReadFile(path)
+ if err != nil {
+ t.Fatalf("read %s failed: %v", path, err)
+ }
+ return string(b)
+}
+
+func TestDisableStacktraceTrueNoStackEvenOnError(t *testing.T) {
+ cfg, out := newDevConfigToFile(t)
+ cfg.DisableStacktrace = true // YAML equals to：disableStacktrace: true
+ InitLogger(cfg)
+
+ log := GetLogger()
+ log.Error("boom")
+ _ = log.Sync()
+
+ got := readAll(t, out)
+ line := ""
+ for _, l := range strings.Split(got, "\n") {
+ if strings.Contains(l, "boom") {
+ line = l
+ break
+ }
+ }
+ if line == "" {
+ t.Fatalf("error line not found")
+ }
+ if strings.Contains(strings.ToLower(line), "stacktrace") {
+ t.Fatalf("disableStacktrace=true: must NOT output stacktrace,
got:\n%s", line)
+ }
+}
+
+func TestSetLoggerLevelDoesNotRebuildAndTakesEffect(t *testing.T) {
+ cfg, out := newDevConfigToFile(t)
+ InitLogger(cfg)
+
+ before := GetLogger().SugaredLogger
+
+ // dynamic set to error
+ ok := SetLoggerLevel("error")
+ if !ok {
+ t.Fatalf("SetLoggerLevel returned false")
+ }
+
+ after := GetLogger().SugaredLogger
+ if before != after {
+ t.Fatalf("SetLoggerLevel should NOT rebuild logger; pointer
changed: %p -> %p", before, after)
+ }
+
+ // write new：info should not appear，error should appear
Review Comment:
Mixed language comment contains Chinese characters. Should be English-only:
'write logs: info should not appear, error should appear'
```suggestion
// write new: info should not appear, error should appear
```
##########
pkg/logger/logger_test.go:
##########
@@ -18,66 +18,198 @@
package logger
import (
- "fmt"
+ "os"
"path/filepath"
- "runtime"
+ "strings"
"testing"
+ "time"
)
import (
- "github.com/stretchr/testify/assert"
+ "go.uber.org/zap"
+ "go.uber.org/zap/zapcore"
)
-func TestInitLog(t *testing.T) {
- var (
- err error
- path string
+func newDevConfigToFile(t *testing.T) (*zap.Config, string) {
+ t.Helper()
+ dir := t.TempDir()
+ out := filepath.Join(dir, "zap.log")
+
+ cfg := zap.NewDevelopmentConfig()
+ // output to a temp file
+ cfg.OutputPaths = []string{out}
+ cfg.ErrorOutputPaths = []string{out}
+ cfg.EncoderConfig.StacktraceKey = "stacktrace"
+ cfg.EncoderConfig.CallerKey = "caller"
+
+ return &cfg, out
+}
+
+func readAll(t *testing.T, path string) string {
+ t.Helper()
+ b, err := os.ReadFile(path)
+ if err != nil {
+ t.Fatalf("read %s failed: %v", path, err)
+ }
+ return string(b)
+}
+
+func TestDisableStacktraceTrueNoStackEvenOnError(t *testing.T) {
+ cfg, out := newDevConfigToFile(t)
+ cfg.DisableStacktrace = true // YAML equals to：disableStacktrace: true
+ InitLogger(cfg)
+
+ log := GetLogger()
+ log.Error("boom")
+ _ = log.Sync()
+
+ got := readAll(t, out)
+ line := ""
+ for _, l := range strings.Split(got, "\n") {
+ if strings.Contains(l, "boom") {
+ line = l
+ break
+ }
+ }
+ if line == "" {
+ t.Fatalf("error line not found")
+ }
+ if strings.Contains(strings.ToLower(line), "stacktrace") {
+ t.Fatalf("disableStacktrace=true: must NOT output stacktrace,
got:\n%s", line)
+ }
+}
+
+func TestSetLoggerLevelDoesNotRebuildAndTakesEffect(t *testing.T) {
+ cfg, out := newDevConfigToFile(t)
+ InitLogger(cfg)
+
+ before := GetLogger().SugaredLogger
+
+ // dynamic set to error
+ ok := SetLoggerLevel("error")
+ if !ok {
+ t.Fatalf("SetLoggerLevel returned false")
+ }
+
+ after := GetLogger().SugaredLogger
+ if before != after {
+ t.Fatalf("SetLoggerLevel should NOT rebuild logger; pointer
changed: %p -> %p", before, after)
+ }
+
+ // write new：info should not appear，error should appear
+ log := GetLogger()
+ log.Info("info should be filtered")
+ log.Error("error should appear")
+ _ = log.Sync()
+
+ got := readAll(t, out)
+ if strings.Contains(got, "info should be filtered") {
+ t.Fatalf("info should NOT appear after level set to
error:\n%s", got)
+ }
+ if !strings.Contains(got, "error should appear") {
+ t.Fatalf("error should appear but not found:\n%s", got)
+ }
+}
+
+func TestHotReloadRebuildsAndSwitchesSink(t *testing.T) {
+ // cfg1 -> out1
+ cfg1, out1 := newDevConfigToFile(t)
+ InitLogger(cfg1)
+ l1 := GetLogger().SugaredLogger
+
+ const (
+ H1 = "hello-1"
+ H2 = "hello-2"
)
- err = InitLog("")
- assert.EqualError(t, err, "log configure file name is nil")
-
- path, err = filepath.Abs("./log.xml")
- assert.NoError(t, err)
- err = InitLog(path)
- assert.EqualError(t, err, "log configure file name "+path+" suffix must
be .yml")
-
- path, err = filepath.Abs("./logger.yml")
- assert.NoError(t, err)
- err = InitLog(path)
- var errMsg string
- if runtime.GOOS == "windows" {
- errMsg = fmt.Sprintf("open %s: The system cannot find the file
specified.", path)
- } else {
- errMsg = fmt.Sprintf("open %s: no such file or directory", path)
+ GetLogger().Info(H1)
+ _ = GetLogger().Sync()
+
+ // cfg2 -> out2（new sink）
+ cfg2, out2 := newDevConfigToFile(t)
+ // to split it，set lever to info
Review Comment:
Mixed language comments contain Chinese characters and typo. Should be
English-only: 'cfg2 -> out2 (new sink)' and 'to split it, set level to info'
```suggestion
// to split it, set level to info
```
##########
pkg/logger/controller.go:
##########
@@ -23,33 +23,31 @@ import (
)
import (
- "go.uber.org/zap"
"go.uber.org/zap/zapcore"
)
// logController governs the logging output or configuration changes
throughout the entire project.
type logController struct {
- mu sync.RWMutex
-
+ mu sync.RWMutex
logger *pixiuLogger
}
-// setLoggerLevel safely changes the log level in a concurrent manner.
+// setLoggerLevel changes the level at runtime without rebuilding the logger.
func (c *logController) setLoggerLevel(level string) bool {
- c.mu.Lock()
- defer c.mu.Unlock()
- lvl := c.parseLevel(level)
- if lvl == nil {
+ lvl, ok := c.parseLevel(level)
+ if !ok {
return false
}
-
- c.logger.config.Level = *lvl
- l, _ := c.logger.config.Build(zap.AddCallerSkip(2))
- c.logger = &pixiuLogger{SugaredLogger: l.Sugar(), config:
c.logger.config}
+ c.mu.RLock()
+ defer c.mu.RUnlock()
Review Comment:
Using RLock for a write operation. The code modifies the logger's level via
SetLevel(), which requires a write lock. Change to c.mu.Lock() and
c.mu.Unlock().
```suggestion
c.mu.Lock()
defer c.mu.Unlock()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
