-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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): support customization of log json marshal #18429
feat(log): support customization of log json marshal #18429
Conversation
Walkthrough# Walkthrough
The recent changes aim to enhance the flexibility of JSON marshaling within the logging package. This is achieved by introducing a new field `JSONMarshal` to the `Config` struct, along with a corresponding function to set this field. The `zerolog.InterfaceMarshalFunc` is now conditionally set based on the presence of `logCfg.JSONMarshal`, allowing for custom JSON marshaling of various types. Additionally, the `init` function and `NewLogger` function have been modified to incorporate the new JSON marshaling logic.
# Changes
| File(s) | Change Summary |
| --------------------- | -------------------------------------------------------------------------------------------------------------------- |
| log/CHANGELOG.md | Added support for customization of log JSON marshaling through pull request #18429. |
| log/logger.go | Introduced a new conditional block to set the `zerolog.InterfaceMarshalFunc` based on `logCfg.JSONMarshal`. Modified the `init` and `NewLogger` functions to incorporate the new JSON marshaling logic. |
| log/options.go | Added a new field `JSONMarshal` to the `Config` struct and a corresponding function `JSONMarshalOption` to set this field. Imported the "encoding/json" package and assigned `json.Marshal` to the `JSONMarshal` field in the `defaultConfig` variable. | TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- log/CHANGELOG.md (1 hunks)
- log/logger.go (1 hunks)
- log/options.go (2 hunks)
Additional comments: 5
log/CHANGELOG.md (1)
- 23-24: The changelog entry is clear and concise, providing a link to the pull request for more details. Ensure that the link is correct and leads to the correct pull request.
log/logger.go (1)
- 74-90: The new conditional block allows for custom JSON marshaling of various types including
json.Marshaler
,encoding.TextMarshaler
, andfmt.Stringer
. This enhances the flexibility of JSON marshaling within the logger. However, it's important to ensure that thelogCfg.JSONMarshal
function is thoroughly tested to handle all these types correctly.log/options.go (3)
18-18: The default JSON marshaler is set to
json.Marshal
. This is a sensible default, but it's important to ensure that all types that will be logged are compatible with this marshaler. If a type implements thejson.Marshaler
interface, that will be used instead.29-29: The
JSONMarshal
field is added to theConfig
struct. This allows for custom JSON marshaling functions to be used. This is a good addition for flexibility, but it's important to ensure that the provided function is safe and performs as expected.95-99: The
JSONMarshalOption
function is added. This function allows for theJSONMarshal
field in theConfig
struct to be set. This is a good addition for flexibility, but it's important to ensure that the provided function is safe and performs as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to me. Few nits.
Co-authored-by: Julien Robert <julien@rbrt.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Julien Robert <julien@rbrt.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- log/options.go (2 hunks)
Additional comments: 3
log/options.go (3)
18-18: The default JSON marshaler is set to
json.Marshal
. This is a good default as it is the standard way to marshal JSON in Go. However, it's important to note that this could be overridden by the user, potentially leading to unexpected behavior if the provided function does not properly marshal to JSON.29-29: The
JSONMarshal
field has been added to theConfig
struct. This allows for customization of the JSON marshaling function. This is a good addition as it allows for more flexibility in how JSON is marshaled.95-99: The
JSONMarshalOption
function has been added to set theJSONMarshal
field in theConfig
struct. This is a good addition as it allows for theJSONMarshal
field to be set in a consistent manner with the other options.
Co-authored-by: Julien Robert <julien@rbrt.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- log/logger.go (2 hunks)
- log/options.go (2 hunks)
Additional comments: 4
log/options.go (3)
10-19: The
defaultConfig
has been updated to includeJSONMarshal: json.Marshal
. This is a sensible default as it uses the standard library's JSON marshaling function. Ensure that this default behavior is documented.22-30: A new field
JSONMarshal
has been added to theConfig
struct. This allows for custom JSON marshaling functions to be used. This is a good addition as it increases the flexibility of the logger.94-99: The
JSONMarshalOption
function has been added. This function allows for theJSONMarshal
field in theConfig
struct to be set. This is a good addition as it allows for the use of custom JSON marshaling functions.log/logger.go (1)
- 59-81: The new logic for setting the
jsonMarshaler
and updating thezerolog.InterfaceMarshalFunc
is sound. However, theinit
function mentioned in the previous comment is not present in the provided code. If it exists elsewhere, consider removing it as suggested by the previous reviewer, since the initialization is now handled here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- log/logger.go (2 hunks)
Additional comments: 2
log/logger.go (2)
64-66: The
jsonMarshaler
is being set based on thelogCfg.JSONMarshal
. IflogCfg.JSONMarshal
isnil
,json.Marshal
is used as a default. This is a good practice as it provides a sensible default and allows for customization.67-75: The
InterfaceMarshalFunc
is being set to handle different types of interfaces. This is a good practice as it allows for different types of interfaces to be marshaled correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- log/logger.go (3 hunks)
Additional comments: 1
log/logger.go (1)
- 52-65: The
WithJSONMarshal
function allows the user to specify a custom JSON marshaling function. This is a good addition for flexibility. However, it's important to note that this function changes the globalInterfaceMarshalFunc
ofzerolog
, which could affect other parts of the application that also usezerolog
. Make sure to document this side effect clearly.
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- log/logger.go (3 hunks)
Additional comments: 2
log/logger.go (2)
13-18: The previous comment about
InterfaceMarshalFunc
being set in theinit
function is still valid. Consider moving this to a function that is explicitly called by the user.52-65: The new
WithJSONMarshal
function allows for custom JSON marshaling. Ensure that themarshaler
function passed as an argument is robust and handles all necessary types. Also, consider adding error handling within this function to handle potential failures of themarshaler
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- log/logger.go (3 hunks)
Additional comments: 2
log/logger.go (2)
13-18: The
init
function has been updated to set thezerolog.InterfaceMarshalFunc
to a function that accepts any type and returns a JSON marshaled byte array. This change allows for custom JSON marshaling of various types, enhancing flexibility. However, ensure that the custom marshaling function handles all types that may be passed to it, as the default case simply attempts to marshal the input without any type checking. This could potentially lead to runtime errors if an incompatible type is passed.52-65: The
WithJSONMarshal
function has been added to configure global JSON encoding for zerolog. This function accepts a custom marshaler function and sets thezerolog.InterfaceMarshalFunc
to a function that uses this custom marshaler. This is a good addition as it allows for customization of the JSON marshaling behavior. However, similar to theinit
function, ensure that the custom marshaler function handles all types that may be passed to it.
when a struct implements stringer and json.Marshaller, we should use that one first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- log/logger.go (2 hunks)
Additional comments: 1
log/logger.go (1)
- 17-17: The comment from julienrbrt suggests that there might be some code that was removed and should be brought back. Please verify if this is the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thank you!
Description
support customization json marshal of the log sdk
Author Checklist
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
Summary by CodeRabbit