Fixing additional truncation of Error (flyteorg#478)

* Fixing additional truncation of Error Signed-off-by: Ketan Umare <[email protected]> * added trunc message Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> * lint fix Signed-off-by: Ketan Umare <[email protected]> * fixed generate Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Ketan Umare <[email protected]>
eapolinario · Sep 23, 2022 · f3b0248 · f3b0248
1 parent 0d961dc
commit f3b0248
Show file tree

Hide file tree

Showing 7 changed files with 5 additions and 40 deletions.
diff --git a/flytepropeller/events/event_recorder.go b/flytepropeller/events/event_recorder.go
@@ -14,6 +14,7 @@ import (
 )
 
 const maxErrorMessageLength = 104857600 //100KB
+const truncationIndicator = "... <Message Truncated> ..."
 
 type recordingMetrics struct {
 	EventRecordingFailure           labeled.StopWatch
@@ -85,7 +86,7 @@ func (r *eventRecorder) RecordWorkflowEvent(ctx context.Context, e *event.Workfl
 // the beginning and the end of the message to capture the most relevant information.
 func truncateErrorMessage(err *core.ExecutionError, length int) {
 	if len(err.Message) > length {
-		err.Message = fmt.Sprintf("%s%s", err.Message[:length/2], err.Message[(len(err.Message)-length/2):])
+		err.Message = fmt.Sprintf("%s\n%s\n%s", err.Message[:length/2], truncationIndicator, err.Message[(len(err.Message)-length/2):])
 	}
 }
 

diff --git a/flytepropeller/events/event_recorder_test.go b/flytepropeller/events/event_recorder_test.go
@@ -96,6 +96,6 @@ func TestTruncateErrorMessage(t *testing.T) {
 		}
 
 		truncateErrorMessage(&executionError, length)
-		assert.True(t, len(executionError.Message) <= length)
+		assert.True(t, len(executionError.Message) <= length+len(truncationIndicator)+2)
 	}
 }
diff --git a/flytepropeller/pkg/controller/nodes/task/config/config.go b/flytepropeller/pkg/controller/nodes/task/config/config.go
@@ -29,7 +29,6 @@ var (
 			BaseSecond:  2,
 			MaxDuration: config.Duration{Duration: time.Second * 20},
 		},
-		MaxErrorMessageLength: 2048,
 	}
 
 	section = config.MustRegisterSection(SectionKey, defaultConfig)
@@ -40,7 +39,7 @@ type Config struct {
 	MaxPluginPhaseVersions int32            `json:"max-plugin-phase-versions" pflag:",Maximum number of plugin phase versions allowed for one phase."`
 	BarrierConfig          BarrierConfig    `json:"barrier" pflag:",Config for Barrier implementation"`
 	BackOffConfig          BackOffConfig    `json:"backoff" pflag:",Config for Exponential BackOff implementation"`
-	MaxErrorMessageLength  int              `json:"maxLogMessageLength" pflag:",Max length of error message."`
+	MaxErrorMessageLength  int              `json:"maxLogMessageLength" pflag:",Deprecated!!! Max length of error message."`
 }
 
 type BarrierConfig struct {

diff --git a/flytepropeller/pkg/controller/nodes/task/config/config_flags.go b/flytepropeller/pkg/controller/nodes/task/config/config_flags.go
diff --git a/flytepropeller/pkg/controller/nodes/task/pre_post_execution.go b/flytepropeller/pkg/controller/nodes/task/pre_post_execution.go
@@ -156,10 +156,6 @@ func (t *Handler) ValidateOutputAndCacheAdd(ctx context.Context, nodeID v1alpha1
 		if taskErr.ExecutionError == nil {
 			taskErr.ExecutionError = &core.ExecutionError{Kind: core.ExecutionError_UNKNOWN, Code: "Unknown", Message: "Unknown"}
 		}
-		// Errors can be arbitrary long since they are written by containers/potentially 3rd party plugins. This ensures
-		// the error message length will never be big enough to cause write failures to Etcd. or spam Admin DB with huge
-		// objects.
-		taskErr.Message = trimErrorMessage(taskErr.Message, t.cfg.MaxErrorMessageLength)
 		return cacheDisabled, &taskErr, nil
 	}
 

diff --git a/flytepropeller/pkg/controller/nodes/task/transformer.go b/flytepropeller/pkg/controller/nodes/task/transformer.go
@@ -48,14 +48,6 @@ func ToTaskEventPhase(p pluginCore.Phase) core.TaskExecution_Phase {
 	}
 }
 
-func trimErrorMessage(original string, maxLength int) string {
-	if len(original) <= maxLength {
-		return original
-	}
-
-	return original[0:maxLength/2] + original[len(original)-maxLength/2:]
-}
-
 func getParentNodeExecIDForTask(taskExecID *core.TaskExecutionIdentifier, execContext executors.ExecutionContext) (*core.NodeExecutionIdentifier, error) {
 	nodeExecutionID := &core.NodeExecutionIdentifier{
 		ExecutionId: taskExecID.NodeExecutionId.ExecutionId,

diff --git a/flytepropeller/pkg/controller/nodes/task/transformer_test.go b/flytepropeller/pkg/controller/nodes/task/transformer_test.go
@@ -37,29 +37,6 @@ func TestToTaskEventPhase(t *testing.T) {
 	assert.Equal(t, core.TaskExecution_QUEUED, ToTaskEventPhase(pluginCore.PhaseQueued))
 }
 
-func Test_trimErrorMessage(t *testing.T) {
-	const inputStr = "0123456789"
-	t.Run("Length less or equal than max", func(t *testing.T) {
-		input := inputStr
-		assert.Equal(t, input, trimErrorMessage(input, 10))
-	})
-
-	t.Run("Length > max", func(t *testing.T) {
-		input := inputStr
-		assert.Equal(t, "01236789", trimErrorMessage(input, 8))
-	})
-
-	t.Run("Odd Max", func(t *testing.T) {
-		input := inputStr
-		assert.Equal(t, "01236789", trimErrorMessage(input, 9))
-	})
-
-	t.Run("Odd input", func(t *testing.T) {
-		input := "012345678"
-		assert.Equal(t, "012345678", trimErrorMessage(input, 9))
-	})
-}
-
 func TestToTaskExecutionEvent(t *testing.T) {
 	tkID := &core.Identifier{}
 	nodeID := &core.NodeExecutionIdentifier{}