Refactor trace2.Manager to return traceqmnguyen0711/dump-trace2-events-to-logs

The trace2.Manager oversees the incorporation of trace2 and passes on
the resulting trace to managed hooks. Previously, the trace was not
accessible outside of this process, but with this update, the resulting
trace and any errors are returned. This trace can now be used to display
the trace in logs at a later time.

4 files changed, 29 insertions, 23 deletions

diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index dcf5d7c9d..1bcd3ac41 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -602,13 +602,16 @@ func (cf *ExecCommandFactory) GlobalConfiguration(ctx context.Context) ([]Config
 
 func (cf *ExecCommandFactory) trace2Finalizer(manager *trace2.Manager) func(context.Context, *command.Command) {
 	return func(ctx context.Context, cmd *command.Command) {
-		manager.Finish(ctx)
+		trace, err := manager.Finish(ctx)
 		stats := command.StatsFromContext(ctx)
 		if stats != nil {
 			stats.RecordMetadata("trace2.activated", "true")
 			stats.RecordMetadata("trace2.hooks", strings.Join(manager.HookNames(), ","))
-			if manager.Error() != nil {
-				stats.RecordMetadata("trace2.error", manager.Error().Error())
+			if trace != nil {
+				stats.RecordMetadata("trace2.events", trace.Inspect(true))
+			}
+			if err != nil {
+				stats.RecordMetadata("trace2.error", err.Error())
 			}
 		}
 	}
diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go
index f234bbb8c..16df0b056 100644
--- a/internal/git/command_factory_test.go
+++ b/internal/git/command_factory_test.go
@@ -856,6 +856,13 @@ func TestWithTrace2Hooks(t *testing.T) {
 				for key, value := range tc.expectedStats {
 					require.Equal(t, value, fields[key])
 				}
+				if _, exists := tc.expectedStats["trace2.activated"]; exists {
+					loggedEvents := fields["trace2.events"]
+					require.NotNil(t, loggedEvents)
+					for _, event := range essentialEvents {
+						require.Contains(t, loggedEvents, event)
+					}
+				}
 			} else {
 				require.Nil(t, command.StatsFromContext(ctx))
 			}
diff --git a/internal/git/trace2/manager.go b/internal/git/trace2/manager.go
index 229a71dcd..2ab0db10e 100644
--- a/internal/git/trace2/manager.go
+++ b/internal/git/trace2/manager.go
@@ -35,11 +35,6 @@ func (m *Manager) HookNames() []string {
 	return names
 }
 
-// Error returns the error occurs after the manager finishes
-func (m *Manager) Error() error {
-	return m.err
-}
-
 // Inject injects the path to the tempfile used to store trace2 events and conventional environment
 // variables to the input ENV list.
 func (m *Manager) Inject(env []string) []string {
@@ -75,9 +70,9 @@ func (m *Manager) Inject(env []string) []string {
 }
 
 // Finish reads the events, parses them to a tree, triggers hook handlers, and clean up the fd.
-func (m *Manager) Finish(ctx context.Context) {
-	if m.Error() != nil {
-		return
+func (m *Manager) Finish(ctx context.Context) (*Trace, error) {
+	if m.err != nil {
+		return nil, m.err
 	}
 
 	defer func() {
@@ -97,17 +92,15 @@ func (m *Manager) Finish(ctx context.Context) {
 
 	trace, err := Parse(ctx, m.fd)
 	if err != nil {
-		m.err = fmt.Errorf("trace2: parsing events: %w", err)
-		return
+		return nil, fmt.Errorf("trace2: parsing events: %w", err)
 	}
 	if trace == nil {
-		m.err = fmt.Errorf("trace2: no events to handle")
-		return
+		return nil, fmt.Errorf("trace2: no events to handle")
 	}
 	for _, hook := range m.hooks {
 		if err := hook.Handle(ctx, trace); err != nil {
-			m.err = fmt.Errorf("trace2: executing %q handler: %w", hook.Name(), err)
-			return
+			return nil, fmt.Errorf("trace2: executing %q handler: %w", hook.Name(), err)
 		}
 	}
+	return trace, nil
 }
diff --git a/internal/git/trace2/manager_test.go b/internal/git/trace2/manager_test.go
index bdbfd25b0..d84edf627 100644
--- a/internal/git/trace2/manager_test.go
+++ b/internal/git/trace2/manager_test.go
@@ -124,14 +124,15 @@ func TestManager(t *testing.T) {
 				require.Nil(t, manager.fd)
 			}
 
-			manager.Finish(testhelper.Context(t))
-			require.NoError(t, manager.Error())
+			trace, err := manager.Finish(testhelper.Context(t))
+			require.NoError(t, err)
 			if activated {
 				require.NoFileExists(t, manager.fd.Name())
 			}
 			if assert != nil {
 				assert(t, manager)
 			}
+			require.Equal(t, expectedTrace, trace.Inspect(false))
 		})
 	}
 }
@@ -181,10 +182,11 @@ func TestManager_tempfileFailures(t *testing.T) {
 			_ = manager.Inject([]string{})
 
 			tc.setup(t, manager)
-			manager.Finish(testhelper.Context(t))
+			trace, err := manager.Finish(testhelper.Context(t))
 
-			require.Regexp(t, tc.expectedError, manager.Error().Error())
+			require.Regexp(t, tc.expectedError, err.Error())
 			require.NoFileExists(t, manager.fd.Name())
+			require.Nil(t, trace)
 		})
 	}
 }
@@ -215,8 +217,9 @@ func TestManager_handlerFailures(t *testing.T) {
 
 	events := testhelper.MustReadFile(t, "testdata/git-pack-objects.event")
 	require.NoError(t, os.WriteFile(manager.fd.Name(), events, os.ModeAppend))
-	manager.Finish(testhelper.Context(t))
+	trace, err := manager.Finish(testhelper.Context(t))
 
-	require.Equal(t, `trace2: executing "dummy2" handler: something goes wrong`, manager.Error().Error())
+	require.Equal(t, `trace2: executing "dummy2" handler: something goes wrong`, err.Error())
 	require.NoFileExists(t, manager.fd.Name())
+	require.Nil(t, trace)
 }