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

enhance log level and environment #201

Merged
merged 3 commits into from
Feb 17, 2025

Conversation

morvencao
Copy link
Contributor

@morvencao morvencao commented Oct 14, 2024

Support consistent environment settings for maestro agent with maestro server.
Changed default log level to 4, so that cloudevents will be printed for debug.

@morvencao morvencao force-pushed the br_agent_environment branch from 207deb0 to 3437521 Compare October 14, 2024 09:10
@morvencao
Copy link
Contributor Author

e2e is broken by #203

@morvencao morvencao force-pushed the br_agent_environment branch from 3437521 to 486b7fc Compare October 15, 2024 06:34
@morvencao
Copy link
Contributor Author

/assign @clyang82 @skeeey

@morvencao morvencao force-pushed the br_agent_environment branch from 486b7fc to 857d071 Compare October 16, 2024 03:15
- name: KLOG_V
displayName: KLOG V Level
description: Log verbosity level
value: "1"
Copy link
Contributor

Choose a reason for hiding this comment

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

may be 3 by default?

Copy link
Contributor

Choose a reason for hiding this comment

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

and may add more description? e.g. level 1 means error, level 2 means warning ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • updated default KLOG_V environment in template to 3.
  • added comments to explain the mapping between klog level and zap log level.

// Initialize zap logger
zc := zap.NewDevelopmentConfig()
// Initialize zap logger based on environment
var zc zap.Config
Copy link
Contributor

Choose a reason for hiding this comment

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

for zap, it can support dynamically update the log level. that can help us on the troubleshooting. Can we do it instead of update the level in environment variable only? maybe define a configmap for the log level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after several attempts, I found that zap log supports dynamic log level changes, but klog does not.

@morvencao morvencao force-pushed the br_agent_environment branch from 2b44f50 to 22d22d4 Compare February 11, 2025 06:48
@morvencao morvencao changed the title consistent environment settings. enhance log level and environment Feb 11, 2025
zc.Level = zap.NewAtomicLevelAt(zapcore.Level(0 - logLevel))
// Disable stacktrace for production environment
zc.DisableStacktrace = true
Copy link
Contributor

Choose a reason for hiding this comment

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

You may move it to line 47 to reflect your comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the comment.

@morvencao morvencao force-pushed the br_agent_environment branch from 22d22d4 to 8bd95c0 Compare February 17, 2025 04:17
Signed-off-by: morvencao <[email protected]>
@morvencao morvencao force-pushed the br_agent_environment branch from 8bd95c0 to 6fbeb6b Compare February 17, 2025 04:19
Copy link
Contributor

@clyang82 clyang82 left a comment

Choose a reason for hiding this comment

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

LGTM

@clyang82
Copy link
Contributor

/ok-to-test

@clyang82 clyang82 merged commit a40e162 into openshift-online:main Feb 17, 2025
8 checks passed
@morvencao morvencao deleted the br_agent_environment branch February 17, 2025 05:43
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.

3 participants