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 format using zapr logger. #195

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

morvencao
Copy link
Contributor

@morvencao morvencao commented Sep 12, 2024

ref: https://issues.redhat.com/browse/ACM-14209

This PR introduces the following changes:

  • Replaces glog with klog
    Note: Maestro still indirectly depends on glog due to the current dependencies on github.com/openshift-online/ocm-sdk-go and github.com/openshift-online/ocm-common, which use glog.
  • Configures klog to use zapr as the backing logger.
    This setup enables enhanced logging features, including detailed timestamps and structured log output (e.g., JSON format).
  • Refines the flags for the root command and subcommands to ensure they are correctly set.

Before this PR:

$ kc -n maestro logs deploy/maestro | head -3
I0826 16:01:43.167516       1 logger.go:103]  Received event from channel [status_events] : ffc0317d-4f73-4826-b9c9-1cebd3a4197f
I0826 16:01:43.186375       1 logger.go:103]  Broadcast the resource status 0d555480-f93b-57da-aab0-1269547348ac
I0826 16:01:43.236294       1 logger.go:103]  Broadcast the resource status fc396131-dc49-5587-9564-70f49d853a1a

After this PR:

$ kc -n maestro logs deploy/maestro | head -20
2024-09-12T08:40:06.976Z	INFO	environments/framework.go:79	Initializing development environment
2024-09-12T08:40:06.981Z	INFO	environments/framework.go:165	Using Mock OCM Authz Client
2024-09-12T08:40:06.993Z	LEVEL(-3)	cert/cert_rotation.go:56	Starting client certificate rotation controller
2024-09-12T08:40:06.999Z	INFO	clientcmd/client_config.go:659	Neither --kubeconfig nor --master was specified.  Using the inClusterConfig.  This might not work.
2024-09-12T08:40:06.999Z	INFO	environments/framework.go:243	Disabling Sentry error reporting
2024-09-12T08:40:07.000Z	INFO	servecmd/cmd.go:49	Setting up pulse server
2024-09-12T08:40:07.002Z	INFO	server/grpc_server.go:113	Serving gRPC service with TLS at 8090
2024-09-12T08:40:07.002Z	LEVEL(-5)	channelz/logging.go:31	[core] [Server #1]Server created

2024-09-12T08:40:07.002Z	DEBUG	logger/logger.go:103	 Kind controller handling events
2024-09-12T08:40:07.002Z	DEBUG	logger/logger.go:103	 Status controller handling events
2024-09-12T08:40:07.002Z	INFO	server/healthcheck_server.go:53	Serving HealthCheck with TLS at 8083
2024-09-12T08:40:07.002Z	DEBUG	logger/logger.go:103	 Starting event controller
2024-09-12T08:40:07.002Z	DEBUG	logger/logger.go:103	 Starting pulse server
2024-09-12T08:40:07.002Z	LEVEL(-10)	logger/logger.go:103	 Checking liveness of maestro instances
2024-09-12T08:40:07.003Z	INFO	server/api_server.go:148	Serving with TLS at 8000
2024-09-12T08:40:07.003Z	INFO	event/event.go:75	Starting event broadcaster
2024-09-12T08:40:07.003Z	DEBUG	logger/logger.go:103	 Kind controller listening for events
2024-09-12T08:40:07.003Z	DEBUG	logger/logger.go:103	 Status controller listening for status events
2024-09-12T08:40:07.003Z	DEBUG	logger/logger.go:103	 Serving Metrics without TLS at 8080

@morvencao
Copy link
Contributor Author

/assign @qiujian16 @clyang82 @skeeey

@@ -24,18 +23,17 @@ func NewMigrationCommand() *cobra.Command {
}

dbConfig.AddFlags(cmd.PersistentFlags())
cmd.PersistentFlags().AddGoFlagSet(flag.CommandLine)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you remove it? We do not need it anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the flags is added into root command, so that all sub comands can share it.

flags.SetNormalizeFunc(utilflag.WordSepNormalizeFunc)
flags.AddGoFlagSet(flag.CommandLine)
Copy link
Contributor

Choose a reason for hiding this comment

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

where is to configure the logs if you remove it in agent?

Copy link
Contributor Author

@morvencao morvencao Oct 9, 2024

Choose a reason for hiding this comment

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

the flags are added into the root command, we can still configure them when we run sub command(maestro migrate/server/agent).

@clyang82
Copy link
Contributor

clyang82 commented Oct 9, 2024

/ok-to-test

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 clyang82 merged commit 350adc0 into openshift-online:main Oct 9, 2024
7 checks passed
@morvencao morvencao deleted the br_log_format branch October 9, 2024 03:13
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.

2 participants