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

Added new Log Enconder Config #2927

Merged
merged 7 commits into from May 13, 2024

Conversation

yuriolisa
Copy link
Contributor

Description:

Added 5 new parameters to enable a customized JSON log Encoder:
--enable-customized-log-encoder = It will enable the feature to set customized keys and formats to the logs, otherwise, the operator will use the zap-devel config.
--log-message-key = It will replace the Key of the Message field (e.g. "msg")
--log-level-key = It will replace the Key of the Level field (e.g. "severity")
--log-time-key= It will replace the Key of the Time field (e.g. "timestamp")
--log-time-format = It will replace the Format of the Time field (e.g. "epoch", "epochmillis", "epochnano", "iso8601", "rfc3339" or "rfc3339nano")
--log-level-format = It will replace the Format of the Level field (e.g. "uppercase", "lowercase")

Link to tracking Issue(s):

Testing:

Documentation:

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
@yuriolisa yuriolisa requested a review from a team as a code owner May 6, 2024 10:28
Copy link
Contributor

@swiatekm-sumo swiatekm-sumo left a comment

Choose a reason for hiding this comment

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

Some of these can be set via existing flags: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/log/zap#Options.BindFlags. If we need to add more, can we use the existing naming convention?

@yuriolisa
Copy link
Contributor Author

Some of these can be set via existing flags: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/log/zap#Options.BindFlags. If we need to add more, can we use the existing naming convention?

I'm aware of these BindFlags options, but when you set a new Enconderconfig you have to populate these values as well, due to that I've put these parameters.
The name convention is a good option, however, the use case for that issue is to deliver an option to the users to customize their operator logs' fields.

@swiatekm-sumo
Copy link
Contributor

Some of these can be set via existing flags: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/log/zap#Options.BindFlags. If we need to add more, can we use the existing naming convention?

I'm aware of these BindFlags options, but when you set a new Enconderconfig you have to populate these values as well, due to that I've put these parameters. The name convention is a good option, however, the use case for that issue is to deliver an option to the users to customize their operator logs' fields.

I think you can use Options.EncoderConfigOptions to modify the encoder config without overwriting it? Right now, we have a --zap-time-encoding which appears to do the same thing as your --log-time-format. We should either use the existing flag or remove it and use the new one.

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
Signed-off-by: Yuri Sa <yurimsa@gmail.com>
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
Signed-off-by: Yuri Sa <yurimsa@gmail.com>
Signed-off-by: Yuri Sa <yurimsa@gmail.com>
Comment on lines +171 to +174
pflag.StringVar(&encodeMessageKey, "zap-message-key", "message", "The message key to be used in the customized Log Encoder")
pflag.StringVar(&encodeLevelKey, "zap-level-key", "level", "The level key to be used in the customized Log Encoder")
pflag.StringVar(&encodeTimeKey, "zap-time-key", "timestamp", "The time key to be used in the customized Log Encoder")
pflag.StringVar(&encodeLevelFormat, "zap-level-format", "uppercase", "The level format to be used in the customized Log Encoder")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the defaults here the same as we currently have? That is, if we don't set any of these, there's no change in behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but we can't compare it with the current log because we use the console encoder by default, while these changes will be effective when we set the zap-encoder=json.

Copy link
Contributor

@swiatekm-sumo swiatekm-sumo left a comment

Choose a reason for hiding this comment

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

LGTM, modulo the lint failure.

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

@yuriolisa can you also please update some docs too for this change? This would assist with the feedback we got in #873

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀 🎉

@jaronoff97 jaronoff97 merged commit eb44580 into open-telemetry:main May 13, 2024
33 checks passed
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.

Configure zap EncoderConfig for operator
3 participants