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

allow for logging customization #130

Merged
merged 1 commit into from
Feb 27, 2024
Merged

allow for logging customization #130

merged 1 commit into from
Feb 27, 2024

Conversation

nabuskey
Copy link
Collaborator

@nabuskey nabuskey commented Jan 23, 2024

As mentioned in #15, currently it's not possible to configure logging format or level.

This PR ensures logging customization work and implements our own defaults: RFC3339 timestamps and console logs.
Uses slog as the underlying implementation.

@cmoulliard
Copy link
Contributor

Ideally we should use the new go logger - #15 (comment) ;-) @nabuskey

@nabuskey
Copy link
Collaborator Author

I know. I was trying to use slog first but our controller-runtime is too old and doesn't come with a version of logr that supports slog. #95 needs to be resolved first. At least this PR makes it possible to change logging configuration...

Copy link
Contributor

@nimakaviani nimakaviani left a comment

Choose a reason for hiding this comment

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

LGTM.

if we pull in the new Argo types repo that @greghaynes created, shall we be able to update controller-runtime and use the newer logr instead?

@nabuskey
Copy link
Collaborator Author

Yes. We should be able to do that.

@nabuskey nabuskey marked this pull request as draft February 9, 2024 17:25
@nabuskey nabuskey force-pushed the logger branch 2 times, most recently from 4512a58 to 24c7435 Compare February 13, 2024 18:21
@nabuskey
Copy link
Collaborator Author

This PR now uses slog as the underlying logging implementation for controllers.

@nabuskey nabuskey marked this pull request as ready for review February 13, 2024 18:30
@cmoulliard
Copy link
Contributor

cmoulliard commented Feb 14, 2024

That looks better using ./idpbuilder create -l debug

time=2024-02-14T12:45:51.076+01:00 level=INFO msg="Starting EventSource" controller=custompackage controllerGroup=idpbuilder.cnoe.io controllerKind=CustomPackage source="kind source: *v1alpha1.CustomPackage"
time=2024-02-14T12:45:51.076+01:00 level=INFO msg="Starting Controller" controller=custompackage controllerGroup=idpbuilder.cnoe.io controllerKind=CustomPackage
t

Remarks - see: #15 (comment):

  • We should review the format of the time as it is still a bit too long 2024-02-14T12:45:51.076+01:00
	customTimeEncoder := func(t time.Time, enc zapcore.PrimitiveArrayEncoder) {
		enc.AppendString(t.Format("2006-01-02 15:04:05.000"))
	}
  • What about showing colors EncodeLevel: zapcore.CapitalColorLevelEncoder, ?
  • Log fields should be removed time=, level=, msg=
2024-02-14 12:45:51.076 INFO custompackage controllerGroup=idpbuilder.cnoe.io controllerKind=CustomPackage source="kind source: *v1alpha1.CustomPackage"
...

@nabuskey
Copy link
Collaborator Author

Let's worry about formats and coloring and what not later. Like I mentioned, this is meant to allow for actual customization of logs. Right now, we have NO control over log levels or formats.

@greghaynes
Copy link
Contributor

Can we make a good-first-issue for improving this logging as we mention? I think this PR is good to merge as is, but I'd like to not lose that issue if possible.

Signed-off-by: Manabu McCloskey <[email protected]>
@nabuskey nabuskey merged commit dd8abfb into cnoe-io:main Feb 27, 2024
1 of 2 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.

4 participants