Skip to content

modify log format for readability#423

Open
nimakaviani wants to merge 1 commit intocnoe-io:mainfrom
nimakaviani:format-logs
Open

modify log format for readability#423
nimakaviani wants to merge 1 commit intocnoe-io:mainfrom
nimakaviani:format-logs

Conversation

@nimakaviani
Copy link
Copy Markdown
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

fixes cnoe-io#395

Signed-off-by: Nima Kaviani <nkaviani@amazon.com>
@cmoulliard
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@punkwalker
Copy link
Copy Markdown
Contributor

Do we want to continue working on this? I believe we have a good logging format via --color option. WDYT @cmoulliard

@nabuskey
Copy link
Copy Markdown
Collaborator

I think this should be worked on but I don't think Nima has time. I'd say close it and open an issue to track it.

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

4 participants