WIP: [VC-35738] Prototype: Use structured logging everywhere #593
Conversation
wallrj
changed the title
Signed-off-by: Richard Wall <[email protected]>
wallrj
force-pushed
the
VC-35738/klog-1
branch
from
53f70f6 to
6659cd1
Compare
| Long: `The agent will periodically gather data for the configured data | ||
| gatherers and send it to a remote backend for evaluation`, | ||
| Run: agent.Run, | ||
| RunE: agent.Run, |
There was a problem hiding this comment.
I'll break this PR up in to some smaller chunks.
Changing these to RunE allows me to remove all the use of log.Fatal in the code.
There was a problem hiding this comment.
This seems like a good idea.
That said, doing this will systematically show the "usage" section regardless of whether the error is due to a problem with a flag, or if the problem occurred while actually running the command... This problem is documented in spf13/cobra#340. The solution seems to be to set SilenceUsage to true on the root cobra command.
There was a problem hiding this comment.
| serverMux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) | ||
| serverMux.HandleFunc("/debug/pprof/profile", pprof.Profile) | ||
| serverMux.HandleFunc("/debug/pprof/symbol", pprof.Symbol) | ||
| serverMux.HandleFunc("/debug/pprof/trace", pprof.Trace) |
There was a problem hiding this comment.
Another small PR will be to remove this extra pprof server and simply add it to the main server.
There was a problem hiding this comment.
| if err != nil && !errors.Is(err, http.ErrServerClosed) { | ||
| logs.Log.Fatalf("failed to run pprof profiler: %s", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
To remove this particular instance of log.Fatalf I need to replace these unmanaged go routines with a errgroup.Go so that I can catch the errors and return them.
| // The goal is to satisfy some Kubernetes distributions, like OpenShift, | ||
| // that require a liveness and health probe to be present for each pod. |
There was a problem hiding this comment.
Not sure what "health" probe means here.
I thought that only liveness probe was required by OpenShift?
ReadinessProbe makes no sense for venafi-kubernetes-agent.
There was a problem hiding this comment.
My mistake, I confused "health probe" with "readiness probe". It should say "readiness probe" here.
| group, gctx := errgroup.WithContext(ctx) | ||
|
|
||
| defer func() { | ||
| // TODO: replace Fatalf log calls with Errorf and return the error |
| var Log *log.Logger | ||
|
|
||
| func init() { | ||
| Log = slog.NewLogLogger(slog.Default().Handler(), slog.LevelDebug) | ||
| } |
There was a problem hiding this comment.
I can start by just doing this, I think.
Then replacing the and ultimately removing the use of Log
| // Type is only used in help text | ||
| func (e *logFormat) Type() string { | ||
| return "string" | ||
| } |
There was a problem hiding this comment.
This was copied from cert-manager/approver-policy
2 participants
No description provided.