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(zap): add multiple zapcore support #19

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

StellarisW
Copy link
Member

What type of PR is this?

feat

What this PR does / why we need it (English/Chinese):

目前hlogzap扩展不支持使用多个zapcore的场景,比如我既需要输出到控制台,又需要输出到日志文件

Which issue(s) this PR fixes:

@rogerogers
Copy link
Collaborator

rogerogers commented Feb 3, 2023

logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
	return zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.ErrorLevel))
})))

maybe we could use zap.WrapCore custom our zapcore?

@StellarisW
Copy link
Member Author

	logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
		return zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.ErrorLevel))
	})))

maybe we could use zap.WrapCore custom our zapcore?

WithZapOptions only affect struct config so the returned zapcore.Core is useless

@rogerogers
Copy link
Collaborator

l, err := os.OpenFile("log.log", os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0755)
if err != nil {
	log.Fatal(err)
}
logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
	return zapcore.NewTee([]zapcore.Core{
		zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(l), zap.NewAtomicLevelAt(zap.InfoLevel)),
		zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.InfoLevel)),
	}...)
})))

multiple zapcore by NewTee

@StellarisW
Copy link
Member Author

l, err := os.OpenFile("log.log", os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0755)
if err != nil {
	log.Fatal(err)
}
logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
	return zapcore.NewTee([]zapcore.Core{
		zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(l), zap.NewAtomicLevelAt(zap.InfoLevel)),
		zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.InfoLevel)),
	}...)
})))

multiple zapcore by NewTee

it indeed a good way to use multiple zapcores in this way

@rogerogers
Copy link
Collaborator

l, err := os.OpenFile("log.log", os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0755)
if err != nil {
	log.Fatal(err)
}
logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
	return zapcore.NewTee([]zapcore.Core{
		zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(l), zap.NewAtomicLevelAt(zap.InfoLevel)),
		zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.InfoLevel)),
	}...)
})))

multiple zapcore by NewTee

it indeed a good way to use multiple zapcores in this way

We can mention this in our documentation, the code change is not necessary for this repository

@rogerogers
Copy link
Collaborator

logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
    return zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.ErrorLevel))
})))

maybe we could use zap.WrapCore custom our zapcore?

WithZapOptions only affect struct config so the returned zapcore.Core is useless

WrapCore will take effect finally, and will cause setOutput and setLevel not to work

@StellarisW
Copy link
Member Author

logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
    return zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.ErrorLevel))
})))

maybe we could use zap.WrapCore custom our zapcore?

WithZapOptions only affect struct config so the returned zapcore.Core is useless

WrapCore will take effect finally, and will cause setOutput and setLevel not to work

In this case, to solve the warped zapcore.Core probles, we still need change methods SetLevel and SetOutput by NewTee and it will just change the first zapcore.Core in the warped zapcore.Core

@rogerogers
Copy link
Collaborator

logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
    return zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.ErrorLevel))
})))

maybe we could use zap.WrapCore custom our zapcore?

WithZapOptions only affect struct config so the returned zapcore.Core is useless

WrapCore will take effect finally, and will cause setOutput and setLevel not to work

In this case, to solve the warped zapcore.Core probles, we still need change methods SetLevel and SetOutput by NewTee and it will just change the first zapcore.Core in the warped zapcore.Core

It will look weird, and it's not so easy to change the wrapCore behavior, maybe we should avoid using these two func when using wrapCore

@StellarisW
Copy link
Member Author

logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
    return zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.ErrorLevel))
})))

maybe we could use zap.WrapCore custom our zapcore?

WithZapOptions only affect struct config so the returned zapcore.Core is useless

WrapCore will take effect finally, and will cause setOutput and setLevel not to work

In this case, to solve the warped zapcore.Core probles, we still need change methods SetLevel and SetOutput by NewTee and it will just change the first zapcore.Core in the warped zapcore.Core

It will look weird, and it's not so easy to change the wrapCore behavior, maybe we should avoid using these two func when using wrapCore

... how's weeird?

@rogerogers
Copy link
Collaborator

logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
    return zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.ErrorLevel))
})))

maybe we could use zap.WrapCore custom our zapcore?

WithZapOptions only affect struct config so the returned zapcore.Core is useless

WrapCore will take effect finally, and will cause setOutput and setLevel not to work

In this case, to solve the warped zapcore.Core probles, we still need change methods SetLevel and SetOutput by NewTee and it will just change the first zapcore.Core in the warped zapcore.Core

It will look weird, and it's not so easy to change the wrapCore behavior, maybe we should avoid using these two func when using wrapCore

... how's weeird?

Why just change the first...

@StellarisW
Copy link
Member Author

logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
    return zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.ErrorLevel))
})))

maybe we could use zap.WrapCore custom our zapcore?

WithZapOptions only affect struct config so the returned zapcore.Core is useless

WrapCore will take effect finally, and will cause setOutput and setLevel not to work

In this case, to solve the warped zapcore.Core probles, we still need change methods SetLevel and SetOutput by NewTee and it will just change the first zapcore.Core in the warped zapcore.Core

It will look weird, and it's not so easy to change the wrapCore behavior, maybe we should avoid using these two func when using wrapCore

... how's weeird?

Why just change the first...

As It can't specify the certain zapcore.Core, we have to to implement Control interface (just point this case in docs is ok)

@rogerogers
Copy link
Collaborator

logger := hertzzap.NewLogger(hertzzap.WithZapOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
    return zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.AddSync(os.Stdout), zap.NewAtomicLevelAt(zap.ErrorLevel))
})))

maybe we could use zap.WrapCore custom our zapcore?

WithZapOptions only affect struct config so the returned zapcore.Core is useless

WrapCore will take effect finally, and will cause setOutput and setLevel not to work

In this case, to solve the warped zapcore.Core probles, we still need change methods SetLevel and SetOutput by NewTee and it will just change the first zapcore.Core in the warped zapcore.Core

It will look weird, and it's not so easy to change the wrapCore behavior, maybe we should avoid using these two func when using wrapCore

... how's weeird?

Why just change the first...

As It can't specify the certain zapcore.Core, we have to to implement Control interface (just point this case in docs is ok)

sounds ok

@rogerogers
Copy link
Collaborator

Do you join our lark community group or wechat group. contact me.

@StellarisW
Copy link
Member Author

Do you join our lark community group or wechat group. contact me.

你飞书名字是啥

@rogerogers
Copy link
Collaborator

Do you join our lark community group or wechat group. contact me.

你飞书名字是啥

rogerogers

zap/option.go Show resolved Hide resolved
zap/go.mod Outdated Show resolved Hide resolved
zap/README.md Show resolved Hide resolved
zap/option.go Outdated Show resolved Hide resolved
zap/logger_test.go Show resolved Hide resolved
zap/logger_test.go Show resolved Hide resolved
zap/logger_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@rogerogers rogerogers left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

None yet

3 participants