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

fix: ensure consistent JSON log format for automaxprocs #6335

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

omerap12
Copy link

@omerap12 omerap12 commented Nov 15, 2024

Configures automaxprocs to use the application logger instead of default console logging to maintain consistent JSON format across all log output.
output:

keda-admission-webhooks:
 ~/ k logs deployments.apps/keda-admission-webhooks -n keda
2024-11-15T18:39:05Z	INFO	setup	maxprocs: Updating GOMAXPROCS=1: determined from CPU quota	{"system": "automaxprocs"}
2024-11-15T18:39:05Z	INFO	setup	Starting admission webhooks
2024-11-15T18:39:05Z	INFO	setup	KEDA Version: main
2024-11-15T18:39:05Z	INFO	setup	Git Commit: c2a6df04230e1f62d335127e0a02694015c79fa6
2024-11-15T18:39:05Z	INFO	setup	Go Version: go1.23.3
2024-11-15T18:39:05Z	INFO	setup	Go OS/Arch: linux/arm64
2024-11-15T18:39:05Z	INFO	setup	Running on Kubernetes 1.30	{"version": "v1.30.0"}
2024-11-15T18:39:05Z	INFO	controller-runtime.builder	skip registering a mutating webhook, object does not implement admission.Defaulter or WithDefaulter wasn't called	{"GVK": "keda.sh/v1alpha1, Kind=ScaledObject"}

keda-operator:

 ~/ k logs deployment/keda-operator -n keda
2024-11-15T18:39:00Z	INFO	setup	maxprocs: Updating GOMAXPROCS=1: determined from CPU quota	{"system": "automaxprocs"}
2024-11-15T18:39:00Z	INFO	setup	Starting manager
2024-11-15T18:39:00Z	INFO	setup	KEDA Version: main
2024-11-15T18:39:00Z	INFO	setup	Git Commit: c2a6df04230e1f62d335127e0a02694015c79fa6
2024-11-15T18:39:00Z	INFO	setup	Go Version: go1.23.3
2024-11-15T18:39:00Z	INFO	setup	Go OS/Arch: linux/arm64
2024-11-15T18:39:00Z	INFO	setup	Running on Kubernetes 1.30	{"version": "v1.30.0"}
2024-11-15T18:39:01Z	INFO	starting server	{"name": "health probe", "addr": "[::]:8081"}
I1115 18:39:01.071646       1 leaderelection.go:254] attempting to acquire leader lease keda/operator.keda.sh...
I1115 18:39:39.767722       1 leaderelection.go:268] successfully acquired lease keda/operator.keda.sh

keda-operator-metrics-apiserver:

 ~/ k logs deployments.apps/keda-operator-metrics-apiserver -n keda                       
I1115 18:38:55.350590       1 welcome.go:34] "msg"="Starting metrics server" "logger"="keda_metrics_adapter"
I1115 18:38:55.350752       1 welcome.go:35] "msg"="KEDA Version: main" "logger"="keda_metrics_adapter"
I1115 18:38:55.350756       1 welcome.go:36] "msg"="Git Commit: c2a6df04230e1f62d335127e0a02694015c79fa6" "logger"="keda_metrics_adapter"
I1115 18:38:55.350759       1 welcome.go:37] "msg"="Go Version: go1.23.3" "logger"="keda_metrics_adapter"
I1115 18:38:55.350761       1 welcome.go:38] "msg"="Go OS/Arch: linux/arm64" "logger"="keda_metrics_adapter"
I1115 18:38:55.350775       1 welcome.go:39] "msg"="Running on Kubernetes 1.30" "logger"="keda_metrics_adapter" "version"={"major":"1","minor":"30","gitVersion":"v1.30.0","gitCommit":"7c48c2bd72b9bf5c44d21d7338cc7bea77d0ad2a","gitTreeState":"clean","buildDate":"2024-05-13T22:02:25Z","goVersion":"go1.22.2","compiler":"gc","platform":"linux/arm64"}
I1115 18:38:55.350913       1 main.go:263] "msg"="maxprocs: Updating GOMAXPROCS=1: determined from CPU quota" "logger"="keda_metrics_adapter"
I1115 18:38:55.351645       1 main.go:112] "msg"="Connecting Metrics Service gRPC client to the server" "address"="keda-operator.keda.svc.cluster.local:9666" "logger"="keda_metrics_adapter"

Checklist

  • [N/A] When introducing a new scaler, I agree with the scaling governance policy
  • I have verified that my change is according to the deprecations & breaking changes policy
  • [N/A] Tests have been added
  • Changelog has been updated and is aligned with our changelog requirements
  • [N/A] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • [N/A] A PR is opened to update the documentation on (repo) (if applicable)
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #5970

@omerap12 omerap12 requested a review from a team as a code owner November 15, 2024 18:42
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Thanks, this is great!

Could you please define this in a single place and just reference in each component? The best place is probably a new file in https://github.com/kedacore/keda/tree/main/pkg/util

@omerap12
Copy link
Author

Thanks, this is great!

Could you please define this in a single place and just reference in each component? The best place is probably a new file in https://github.com/kedacore/keda/tree/main/pkg/util

fixed :)

@zroubalik
Copy link
Member

zroubalik commented Nov 19, 2024

/run-e2e internal
Update: You can check the progress here

@zroubalik
Copy link
Member

@omerap12 could you please also update Changelog? https://github.com/kedacore/keda/blob/main/CHANGELOG.md#improvements-1

@omerap12
Copy link
Author

@omerap12 could you please also update Changelog? https://github.com/kedacore/keda/blob/main/CHANGELOG.md#improvements-1

Done

err = kedautil.ConfigureMaxProcs(logger)
if err != nil {
logger.Error(err, "failed to set max procs")
return
Copy link
Member

Choose a reason for hiding this comment

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

nit: should this also have exit code 1 to be consistent with other components?

Suggested change
return
os.Exit(1)

Copy link
Author

Choose a reason for hiding this comment

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

I noticed that all the errors are simply returned, like: here. so I chose not to modify the existing error-handling behavior.

Copy link
Member

Choose a reason for hiding this comment

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

sure, just wondering if it makes sense to unify the behavior

keda/cmd/operator/main.go

Lines 118 to 122 in 0a1d8be

err := kedautil.ConfigureMaxProcs(setupLog)
if err != nil {
setupLog.Error(err, "failed to set max procs")
os.Exit(1)
}

err := kedautil.ConfigureMaxProcs(setupLog)
if err != nil {
setupLog.Error(err, "failed to set max procs")
os.Exit(1)
}

but it's just a cosmetic change, I'm ok with either path

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.

Automaxprocs library prints log in a different format then the rest of the KEDA operator
3 participants