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

modify log format for readability #423

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

Conversation

nimakaviani
Copy link
Contributor

@nimakaviani nimakaviani commented Oct 21, 2024

fixes #395

This breaks the log lines into multi-lines for ease of readability.

It does it for both when the color option is enabled and not. We can ideally modify it to be enabled only when the color option is enabled.

cc @cmoulliard

image

@cmoulliard
Copy link
Contributor

We can do that even if I never saw a project logging messages like that in many years

The main issue here is the fact that we log non required messages as controller=localbuild , controllerGroup=, controllerKind= etc. So having multi lines will not help.

Messages should be logged as by examples what k8s API server reports:

W1021 07:36:39.587369       1 reflector.go:547] storage/cacher.go:/tekton.dev/taskruns: failed to list tekton.dev/v1beta1, Kind=TaskRun: conversion webhook for tekton.dev/v1, Kind=TaskRun failed: Post "https:/
/tekton-pipelines-webhook.tekton-pipelines.svc:443/resource-conversion?timeout=30s": tls: failed to verify certificate: x509: certificate signed by unknown authority (possibly because of "x509: ECDSA verificat
ion failure" while trying to verify candidate authority certificate "tekton-pipelines-webhook.tekton-pipelines.svc")

E1021 07:36:39.587423       1 cacher.go:475] cacher (taskruns.tekton.dev): unexpected ListAndWatch error: failed to list tekton.dev/v1beta1, Kind=TaskRun: conversion webhook for tekton.dev/v1, Kind=TaskRun fai
led: Post "https://tekton-pipelines-webhook.tekton-pipelines.svc:443/resource-conversion?timeout=30s": tls: failed to verify certificate: x509: certificate signed by unknown authority (possibly because of "x50
9: ECDSA verification failure" while trying to verify candidate authority certificate "tekton-pipelines-webhook.tekton-pipelines.svc"); reinitializing...

Ideally when there is an the stack trace should be logged on several lines

@nabuskey
Copy link
Collaborator

The logging format is fine for a Kubernetes controller, but idpbuilder is supposed to be a CLI tool. So we should remove irrelevant fields like Charles said. I think we should have these for debug messages though.

@nimakaviani
Copy link
Contributor Author

nimakaviani commented Oct 21, 2024

it could be that we modify this in two ways:

  • provide options for the controller attributions to happen during debug
  • for the multiline printing to happen only with color. For anyone with the intention to run this as part of a DevOps workflow, they wont need color or multiline I suppose.

multiline massively helps with readability though, so I suggest we keep it when color outputs are printed.

@cmoulliard
Copy link
Contributor

Here is a good example about what we should try to achieve (= log messages of kestra)
Screenshot 2024-10-22 at 09 25 12

@nabuskey
Copy link
Collaborator

I'm not really a fun of multi line logging tbh, but I think it's fine if used with the color option because it's meant for humans.

We should definitely get rid of controller name, kind, and reconcile id for info level.

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.

[Suggestion]: Improve the format of the messages logged
3 participants