From e9f8535ca3efb74fa80c0615ceb15813bbbe2de8 Mon Sep 17 00:00:00 2001 From: DavidSeptimus-Klotho Date: Fri, 16 Aug 2024 13:44:02 -0600 Subject: [PATCH] Improves error reporting - In concise mode, errors are rendered cleanly at the end of the terminal output - Silenced cobra error logging to avoid breaking our TUI output - Added error detection for language host startup - Added error messages to clarify issues with the klotho sdk installation --- cmd/klotho/down.go | 14 ++++++--- cmd/klotho/main.go | 5 ++-- pkg/k2/language_host/ir_service.go | 16 ++++++---- pkg/k2/language_host/language_host.go | 30 ++++++++++++++----- .../python/python_language_host.py | 29 +++++++++++++++--- pkg/tui/log_core.go | 10 ++++++- pkg/tui/model.go | 8 ++++- pkg/tui/program.go | 4 +++ pkg/tui/verbosity.go | 12 ++------ 9 files changed, 95 insertions(+), 33 deletions(-) diff --git a/cmd/klotho/down.go b/cmd/klotho/down.go index e41e1c5f..41bae14f 100644 --- a/cmd/klotho/down.go +++ b/cmd/klotho/down.go @@ -43,7 +43,7 @@ func newDownCmd() *cobra.Command { } func getProjectPath(ctx context.Context, inputPath string) (string, error) { - langHost, addr, err := language_host.StartPythonClient(ctx, language_host.DebugConfig{ + langHost, srvState, err := language_host.StartPythonClient(ctx, language_host.DebugConfig{ Port: downConfig.debugPort, Mode: downConfig.debugMode, }, filepath.Dir(inputPath)) @@ -62,15 +62,21 @@ func getProjectPath(ctx context.Context, inputPath string) (string, error) { log.Debug("Waiting for Python server to start") if downConfig.debugMode != "" { // Don't add a timeout in case there are breakpoints in the language host before an address is printed - <-addr.HasAddr + select { + case <-srvState.HasAddr: + case <-srvState.HasErr: + return "", srvState.Error + } } else { select { - case <-addr.HasAddr: + case <-srvState.HasAddr: case <-time.After(30 * time.Second): return "", errors.New("timeout waiting for Python server to start") + case <-srvState.HasErr: + return "", srvState.Error } } - conn, err := grpc.NewClient(addr.Address, grpc.WithTransportCredentials(insecure.NewCredentials())) + conn, err := grpc.NewClient(srvState.Address, grpc.WithTransportCredentials(insecure.NewCredentials())) if err != nil { return "", fmt.Errorf("failed to connect to Python server: %w", err) } diff --git a/cmd/klotho/main.go b/cmd/klotho/main.go index 5bbd1de5..0ec5b3e7 100644 --- a/cmd/klotho/main.go +++ b/cmd/klotho/main.go @@ -27,8 +27,9 @@ func cli() int { }() var rootCmd = &cobra.Command{ - Use: "app", - SilenceUsage: true, + Use: "app", + SilenceUsage: true, + SilenceErrors: true, } rootCmd.PersistentPreRun = func(cmd *cobra.Command, args []string) { diff --git a/pkg/k2/language_host/ir_service.go b/pkg/k2/language_host/ir_service.go index 8d383b4b..7356847e 100644 --- a/pkg/k2/language_host/ir_service.go +++ b/pkg/k2/language_host/ir_service.go @@ -26,24 +26,30 @@ func (irs *LanguageHost) Start(ctx context.Context, debug DebugConfig, pythonPat irs.debugCfg = debug - var addr *ServerAddress - irs.langHost, addr, err = StartPythonClient(ctx, debug, pythonPath) + var srvState *ServerState + irs.langHost, srvState, err = StartPythonClient(ctx, debug, pythonPath) if err != nil { return } log.Debug("Waiting for Python server to start") if debug.Enabled() { // Don't add a timeout in case there are breakpoints in the language host before an address is printed - <-addr.HasAddr + select { + case <-srvState.HasAddr: + case <-srvState.HasErr: + return srvState.Error + } } else { select { - case <-addr.HasAddr: + case <-srvState.HasAddr: case <-time.After(30 * time.Second): return errors.New("timeout waiting for Python server to start") + case <-srvState.HasErr: + return srvState.Error } } - irs.conn, err = grpc.NewClient(addr.Address, grpc.WithTransportCredentials(insecure.NewCredentials())) + irs.conn, err = grpc.NewClient(srvState.Address, grpc.WithTransportCredentials(insecure.NewCredentials())) if err != nil { return fmt.Errorf("failed to connect to Python server: %w", err) } diff --git a/pkg/k2/language_host/language_host.go b/pkg/k2/language_host/language_host.go index cad45bc9..9069df5e 100644 --- a/pkg/k2/language_host/language_host.go +++ b/pkg/k2/language_host/language_host.go @@ -3,11 +3,13 @@ package language_host import ( "context" _ "embed" + "errors" "fmt" "io" "os" "os/exec" "regexp" + "strings" "syscall" "github.com/klothoplatform/klotho/pkg/command" @@ -19,15 +21,26 @@ import ( //go:embed python/python_language_host.py var pythonLanguageHost string -type ServerAddress struct { +type ServerState struct { Log *zap.SugaredLogger Address string + Error error + HasErr chan error HasAddr chan struct{} } +func NewServerState(log *zap.SugaredLogger) *ServerState { + return &ServerState{ + Log: log, + HasErr: make(chan error), + HasAddr: make(chan struct{}), + } +} + var listenOnPattern = regexp.MustCompile(`(?m)^\s*Listening on (\S+)$`) +var exceptionPattern = regexp.MustCompile(`(?s)(?:^|\n)\s*Exception occurred: (.+)$`) -func (f *ServerAddress) Write(b []byte) (int, error) { +func (f *ServerState) Write(b []byte) (int, error) { if f.Address != "" { return len(b), nil } @@ -39,6 +52,12 @@ func (f *ServerAddress) Write(b []byte) (int, error) { f.Log.Debugf("Found language host listening on %s", f.Address) close(f.HasAddr) } + matches = exceptionPattern.FindStringSubmatch(s) + if len(matches) >= 2 { + f.Error = errors.New(strings.TrimSpace(matches[1])) + close(f.HasErr) + close(f.HasAddr) + } return len(b), nil } @@ -66,7 +85,7 @@ func copyToTempDir(name, content string) (string, error) { } -func StartPythonClient(ctx context.Context, debugConfig DebugConfig, pythonPath string) (*exec.Cmd, *ServerAddress, error) { +func StartPythonClient(ctx context.Context, debugConfig DebugConfig, pythonPath string) (*exec.Cmd, *ServerState, error) { log := logging.GetLogger(ctx).Sugar() hostPath, err := copyToTempDir("python_language_host", pythonLanguageHost) if err != nil { @@ -97,10 +116,7 @@ func StartPythonClient(ctx context.Context, debugConfig DebugConfig, pythonPath } cmd.Env = append(cmd.Env, "PYTHONPATH="+pythonPath) - lf := &ServerAddress{ - Log: log, - HasAddr: make(chan struct{}), - } + lf := NewServerState(log) cmd.Stdout = io.MultiWriter(cmd.Stdout, lf) command.SetProcAttr(cmd) diff --git a/pkg/k2/language_host/python/python_language_host.py b/pkg/k2/language_host/python/python_language_host.py index 1f27d1f1..a0824826 100644 --- a/pkg/k2/language_host/python/python_language_host.py +++ b/pkg/k2/language_host/python/python_language_host.py @@ -4,18 +4,38 @@ from concurrent import futures from enum import Enum from time import sleep +import importlib.util +from typing import Optional import grpc import yaml -from klotho import service_pb2, service_pb2_grpc -from klotho.runtime import instance as runtime - class DebugMode(Enum): NONE = 0 VSCODE = 1 INTELLIJ = 2 +class CLIErrorReporter: + def __init__(self, error_message: Optional[str] = None): + self.error_message = error_message + def __enter__(self): + pass + + def __exit__(self, exc_type, exc_value, traceback): + message = self.error_message if self.error_message else (f"Exception occurred: {exc_type.__name__}: {exc_value}" if exc_type is not None else "The language host exited unexpectedly.") + if exc_type is not None: + print(f"Exception occurred: {message}", flush=True) + +with CLIErrorReporter("The Klotho Python IaC SDK is not installed. Please install it by running `pipenv install klotho`."): + spec = importlib.util.find_spec("klotho") + if spec is None: + raise ModuleNotFoundError + +with CLIErrorReporter("The Klotho Python IaC SDK could not be imported. Please check the installation."): + # TODO: Add a check for the minimum required version of the SDK + from klotho import service_pb2, service_pb2_grpc + from klotho.runtime import instance as runtime + def configure_debugging(port: int, mode: DebugMode = DebugMode.VSCODE): if mode == DebugMode.VSCODE: @@ -113,4 +133,5 @@ def signal_handler(sig, frame): if __name__ == "__main__": - cli() + with CLIErrorReporter("The Python language host exited unexpectedly."): + cli() diff --git a/pkg/tui/log_core.go b/pkg/tui/log_core.go index 7da52249..d9604107 100644 --- a/pkg/tui/log_core.go +++ b/pkg/tui/log_core.go @@ -47,7 +47,7 @@ func (c *LogCore) With(f []zapcore.Field) zapcore.Core { if c.verbosity.CombineLogs() { field.AddTo(nc.enc) } - // else (if the field is the construct and we're not combining logs) don't add it to the encoder + // else (if the field is the construct and we're not combining errors) don't add it to the encoder // because the log lines will already be in its own construct section of the output. } else { field.AddTo(nc.enc) @@ -93,6 +93,14 @@ func (c *LogCore) Write(ent zapcore.Entry, fields []zapcore.Field) error { s := buf.String() s = strings.TrimSuffix(s, "\n") + if c.construct == "" && zapcore.ErrorLevel.Enabled(ent.Level) { + c.program.Send(ErrorMessage{ + Message: s, + }) + buf.Free() + return nil + } + c.program.Send(LogMessage{ Construct: construct, Message: s, diff --git a/pkg/tui/model.go b/pkg/tui/model.go index a15ab96f..c0e999a9 100644 --- a/pkg/tui/model.go +++ b/pkg/tui/model.go @@ -23,6 +23,7 @@ type ( consoleWidth int constructWidth int statusWidth int + errors []string } constructModel struct { @@ -81,7 +82,8 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { cm := m.constructModel(msg.Construct) cm.logs.Push(msg.Message) } - + case ErrorMessage: + m.errors = append(m.errors, msg.Message) case UpdateMessage: cm := m.constructModel(msg.Construct) @@ -195,6 +197,10 @@ func (m *model) viewCompact() string { } } } + for _, log := range m.errors { + sb.WriteString(log) + sb.WriteRune('\n') + } return sb.String() } diff --git a/pkg/tui/program.go b/pkg/tui/program.go index f6465d56..16cf5a55 100644 --- a/pkg/tui/program.go +++ b/pkg/tui/program.go @@ -27,6 +27,10 @@ type ( Value any } + ErrorMessage struct { + Message string + } + TuiProgress struct { Prog *tea.Program Construct string diff --git a/pkg/tui/verbosity.go b/pkg/tui/verbosity.go index a4672903..54261dda 100644 --- a/pkg/tui/verbosity.go +++ b/pkg/tui/verbosity.go @@ -27,14 +27,8 @@ func (v Verbosity) LogLevel() zapcore.Level { } } -// CombineLogs controls whether to show all logs comingled in the TUI. In other words, -// sorted by timestamp, not grouped by construct. +// CombineLogs controls whether to show all errors commingled in the TUI. +// In other words, sorted by timestamp, not grouped by construct. func (v Verbosity) CombineLogs() bool { - switch v { - case VerbosityConcise, VerbosityDebugMore: - return true - - default: - return false - } + return VerbosityDebugMore == v }