From 39368d511a2535c19867fd8f528f5380a5392f22 Mon Sep 17 00:00:00 2001 From: James Read Date: Wed, 19 Feb 2025 23:17:06 +0000 Subject: [PATCH] feature: Limit log history to prevent browser lag and grpc encode failures (#507) * feature: Limit log history to prevent browser lag and grpc encode failures * cicd: Simplify if/else chain to switch * cicd: Lint failure in JS because of a trailing space * cicd: Coderabbit fixes on PR * cicd: Move config sanitize into correct place * cicd: Handle negative startOffset, negative endOffset and empty logs * Update internal/executor/executor.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * cicd: Fix getLogs segfault * cicd: Fix code rabbit nitpicks --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- OliveTin.proto | 7 ++- internal/config/config.go | 2 + internal/config/config_reloader.go | 8 ---- internal/config/sanitize.go | 22 ++++++++++ internal/executor/executor.go | 64 ++++++++++++++++++++++++--- internal/executor/executor_test.go | 70 ++++++++++++++++++++++++++++++ internal/grpcapi/grpcApi.go | 14 +++--- webui.dev/index.html | 6 ++- webui.dev/js/marshaller.js | 13 ++++++ 9 files changed, 180 insertions(+), 26 deletions(-) diff --git a/OliveTin.proto b/OliveTin.proto index f3979ad..4beee8b 100644 --- a/OliveTin.proto +++ b/OliveTin.proto @@ -102,7 +102,9 @@ message StartActionByGetAndWaitResponse { LogEntry log_entry = 1; } -message GetLogsRequest{}; +message GetLogsRequest{ + int64 start_offset = 1; +}; message LogEntry { string datetime_started = 1; @@ -120,10 +122,13 @@ message LogEntry { bool execution_started = 14; bool execution_finished = 15; bool blocked = 16; + int64 datetime_index = 17; } message GetLogsResponse { repeated LogEntry logs = 1; + int64 count_remaining = 2; + int64 page_size = 3; } message ValidateArgumentTypeRequest { diff --git a/internal/config/config.go b/internal/config/config.go index a74fadc..14d5b55 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -96,6 +96,7 @@ type Config struct { ExternalRestAddress string LogLevel string LogDebugOptions LogDebugOptions + LogHistoryPageSize int64 Actions []*Action `mapstructure:"actions"` Entities []*EntityFile `mapstructure:"entities"` Dashboards []*DashboardComponent `mapstructure:"dashboards"` @@ -212,6 +213,7 @@ func DefaultConfigWithBasePort(basePort int) *Config { config.EnableCustomJs = false config.ExternalRestAddress = "." config.LogLevel = "INFO" + config.LogHistoryPageSize = 10 config.CheckForUpdates = false config.DefaultPermissions.Exec = true config.DefaultPermissions.View = true diff --git a/internal/config/config_reloader.go b/internal/config/config_reloader.go index a536605..0a34add 100644 --- a/internal/config/config_reloader.go +++ b/internal/config/config_reloader.go @@ -36,14 +36,6 @@ func Reload(cfg *Config) { os.Exit(1) } - if cfg.AuthRequireGuestsToLogin { - log.Infof("AuthRequireGuestsToLogin is enabled. All defaultPermissions will be set to false") - - cfg.DefaultPermissions.View = false - cfg.DefaultPermissions.Exec = false - cfg.DefaultPermissions.Logs = false - } - metricConfigReloadedCount.Inc() metricConfigActionCount.Set(float64(len(cfg.Actions))) diff --git a/internal/config/sanitize.go b/internal/config/sanitize.go index 029f0e0..644fc17 100644 --- a/internal/config/sanitize.go +++ b/internal/config/sanitize.go @@ -41,6 +41,28 @@ func (action *Action) sanitize(cfg *Config) { for idx := range action.Arguments { action.Arguments[idx].sanitize() } + + sanitizeAuthRequireGuestsToLogin(cfg) + sanitizeLogHistoryPageSize(cfg) +} + +func sanitizeAuthRequireGuestsToLogin(cfg *Config) { + if cfg.AuthRequireGuestsToLogin { + log.Infof("AuthRequireGuestsToLogin is enabled. All defaultPermissions will be set to false") + + cfg.DefaultPermissions.View = false + cfg.DefaultPermissions.Exec = false + cfg.DefaultPermissions.Logs = false + } +} + +func sanitizeLogHistoryPageSize(cfg *Config) { + if cfg.LogHistoryPageSize < 10 { + log.Warnf("LogsHistoryLimit is too low, setting it to 10") + cfg.LogHistoryPageSize = 10 + } else if cfg.LogHistoryPageSize > 100 { + log.Warnf("LogsHistoryLimit is high, you can do this, but expect browser lag.") + } } func getActionID(action *Action) string { diff --git a/internal/executor/executor.go b/internal/executor/executor.go index 27b106f..e596e7c 100644 --- a/internal/executor/executor.go +++ b/internal/executor/executor.go @@ -37,8 +37,9 @@ type ActionBinding struct { // Executor represents a helper class for executing commands. It's main method // is ExecRequest type Executor struct { - logs map[string]*InternalLogEntry - LogsByActionId map[string][]*InternalLogEntry + logs map[string]*InternalLogEntry + logsTrackingIdsByDate []string + LogsByActionId map[string][]*InternalLogEntry logmutex sync.RWMutex @@ -85,6 +86,7 @@ type InternalLogEntry struct { ExecutionTrackingID string Process *os.Process Username string + Index int64 /* The following 3 properties are obviously on Action normally, but it's useful @@ -104,6 +106,7 @@ func DefaultExecutor(cfg *config.Config) *Executor { e := Executor{} e.Cfg = cfg e.logs = make(map[string]*InternalLogEntry) + e.logsTrackingIdsByDate = make([]string, 0) e.LogsByActionId = make(map[string][]*InternalLogEntry) e.MapActionIdToBinding = make(map[string]*ActionBinding) @@ -135,18 +138,62 @@ func (e *Executor) AddListener(m listener) { e.listeners = append(e.listeners, m) } -func (e *Executor) GetLogsCopy() map[string]*InternalLogEntry { +// getPagingStartIndex calculates the starting index for log pagination. +// Parameters: +// +// startOffset: The offset from the most recent log (0 means start from the most recent) +// totalLogCount: Total number of logs available +// count: Number of logs to retrieve +// +// Returns: The calculated starting index for pagination +func getPagingStartIndex(startOffset int64, totalLogCount int64, count int64) int64 { + var startIndex int64 + + if startOffset <= 0 { + startIndex = totalLogCount + } else { + startIndex = (totalLogCount - startOffset) + + if startIndex < 0 { + startIndex = 1 + } + } + + return startIndex - 1 +} + +func (e *Executor) GetLogTrackingIds(startOffset int64, pageCount int64) ([]*InternalLogEntry, int64) { e.logmutex.RLock() - copy := make(map[string]*InternalLogEntry) + totalLogCount := int64(len(e.logsTrackingIdsByDate)) - for k, v := range e.logs { - copy[k] = v + startIndex := getPagingStartIndex(startOffset, totalLogCount, pageCount) + + pageCount = min(totalLogCount, pageCount) + + endIndex := max(0, (startIndex-pageCount)+1) + + log.WithFields(log.Fields{ + "startOffset": startOffset, + "pageCount": pageCount, + "total": totalLogCount, + "startIndex": startIndex, + "endIndex": endIndex, + }).Infof("GetLogTrackingIds") + + trackingIds := make([]*InternalLogEntry, 0, pageCount) + + if totalLogCount > 0 { + for i := endIndex; i <= startIndex; i++ { + trackingIds = append(trackingIds, e.logs[e.logsTrackingIdsByDate[i]]) + } } e.logmutex.RUnlock() - return copy + remainingLogs := endIndex + + return trackingIds, remainingLogs } func (e *Executor) GetLog(trackingID string) (*InternalLogEntry, bool) { @@ -176,7 +223,10 @@ func (e *Executor) GetLogsByActionId(actionId string) []*InternalLogEntry { func (e *Executor) SetLog(trackingID string, entry *InternalLogEntry) { e.logmutex.Lock() + entry.Index = int64(len(e.logsTrackingIdsByDate)) + e.logs[trackingID] = entry + e.logsTrackingIdsByDate = append(e.logsTrackingIdsByDate, trackingID) e.logmutex.Unlock() } diff --git a/internal/executor/executor_test.go b/internal/executor/executor_test.go index 8fe955d..45f5f98 100644 --- a/internal/executor/executor_test.go +++ b/internal/executor/executor_test.go @@ -109,3 +109,73 @@ func TestArgumentNameSnakeCase(t *testing.T) { assert.Equal(t, "echo 'Tickling Fred'", out) assert.Nil(t, err) } + +func TestGetLogsEmpty(t *testing.T) { + e, cfg := testingExecutor() + + assert.Equal(t, int64(10), cfg.LogHistoryPageSize, "Logs page size should be 10") + + logs, remaining := e.GetLogTrackingIds(0, 10) + + assert.NotNil(t, logs, "Logs should not be nil") + assert.Equal(t, 0, len(logs), "No logs yet") + assert.Equal(t, int64(0), remaining, "There should be no remaining logs") +} + +func TestGetLogsLessThanPageSize(t *testing.T) { + e, cfg := testingExecutor() + + cfg.Actions = append(cfg.Actions, &config.Action{ + Title: "blat", + Shell: "date", + }) + + assert.Equal(t, int64(10), cfg.LogHistoryPageSize, "Logs page size should be 10") + + logEntries, remaining := e.GetLogTrackingIds(0, 10) + + assert.Equal(t, 0, len(logEntries), "There should be 0 logs") + assert.Zero(t, remaining, "There should be no remaining logs") + + execNewReqAndWait(e, "blat", cfg) + execNewReqAndWait(e, "blat", cfg) + execNewReqAndWait(e, "blat", cfg) + execNewReqAndWait(e, "blat", cfg) + execNewReqAndWait(e, "blat", cfg) + execNewReqAndWait(e, "blat", cfg) + execNewReqAndWait(e, "blat", cfg) + + logEntries, remaining = e.GetLogTrackingIds(0, 10) + + assert.Equal(t, 7, len(logEntries), "There should be 7 logs") + assert.Zero(t, remaining, "There should be no remaining logs") + + execNewReqAndWait(e, "blat", cfg) + execNewReqAndWait(e, "blat", cfg) + execNewReqAndWait(e, "blat", cfg) + execNewReqAndWait(e, "blat", cfg) + execNewReqAndWait(e, "blat", cfg) + + logEntries, remaining = e.GetLogTrackingIds(0, 10) + + assert.Equal(t, 10, len(logEntries), "There should be 10 logs") + assert.Equal(t, int64(2), remaining, "There should be 1 remaining logs") +} + +func execNewReqAndWait(e *Executor, title string, cfg *config.Config) { + req := &ExecutionRequest{ + ActionTitle: title, + Cfg: cfg, + } + + wg, _ := e.ExecRequest(req) + wg.Wait() +} + +func TestGetPagingIndexes(t *testing.T) { + assert.Zero(t, getPagingStartIndex(5, 0, 5), "Testing start index from empty list") + assert.Equal(t, int64(4), getPagingStartIndex(5, 10, 5), "Testing start index from mid point") + assert.Equal(t, int64(9), getPagingStartIndex(-1, 10, 5), "Testing start index with negative offset") + assert.Equal(t, int64(0), getPagingStartIndex(15, 10, 5), "Testing start index with large offset") + assert.Equal(t, int64(9), getPagingStartIndex(0, 10, 0), "Testing start index with zero count") +} diff --git a/internal/grpcapi/grpcApi.go b/internal/grpcapi/grpcApi.go index e0820b8..d408d0f 100644 --- a/internal/grpcapi/grpcApi.go +++ b/internal/grpcapi/grpcApi.go @@ -14,7 +14,6 @@ import ( "errors" "net" - "sort" acl "github.com/OliveTin/OliveTin/internal/acl" config "github.com/OliveTin/OliveTin/internal/config" @@ -209,6 +208,7 @@ func internalLogEntryToPb(logEntry *executor.InternalLogEntry) *pb.LogEntry { ActionId: logEntry.ActionId, DatetimeStarted: logEntry.DatetimeStarted.Format("2006-01-02 15:04:05"), DatetimeFinished: logEntry.DatetimeFinished.Format("2006-01-02 15:04:05"), + DatetimeIndex: logEntry.Index, Output: logEntry.Output, TimedOut: logEntry.TimedOut, Blocked: logEntry.Blocked, @@ -335,24 +335,20 @@ func (api *oliveTinAPI) GetLogs(ctx ctx.Context, req *pb.GetLogsRequest) (*pb.Ge ret := &pb.GetLogsResponse{} - // TODO Limit to 10 entries or something to prevent browser lag. + logEntries, countRemaining := api.executor.GetLogTrackingIds(req.StartOffset, cfg.LogHistoryPageSize) - for trackingId, logEntry := range api.executor.GetLogsCopy() { + for _, logEntry := range logEntries { action := cfg.FindAction(logEntry.ActionTitle) if action == nil || acl.IsAllowedLogs(cfg, user, action) { pbLogEntry := internalLogEntryToPb(logEntry) - pbLogEntry.ExecutionTrackingId = trackingId ret.Logs = append(ret.Logs, pbLogEntry) } } - sorter := func(i, j int) bool { - return ret.Logs[i].DatetimeStarted < ret.Logs[j].DatetimeStarted - } - - sort.Slice(ret.Logs, sorter) + ret.CountRemaining = countRemaining + ret.PageSize = cfg.LogHistoryPageSize return ret, nil } diff --git a/webui.dev/index.html b/webui.dev/index.html index a3cf646..bb38eda 100644 --- a/webui.dev/index.html +++ b/webui.dev/index.html @@ -63,7 +63,7 @@ - +
@@ -74,6 +74,10 @@ + +

There are no logs to display. Return to index

+ +

Note: The server is configured to only send ? log entries at a time. The search box at the top of this page only searches this current page of logs.