diff options
author | Quang-Minh Nguyen <qmnguyen@gitlab.com> | 2023-03-02 11:10:48 +0300 |
---|---|---|
committer | Quang-Minh Nguyen <qmnguyen@gitlab.com> | 2023-03-15 13:06:33 +0300 |
commit | d4c090d0100aed63563add9e92645bccb5c9b399 (patch) | |
tree | d727ffad7a7c7e5f91849135479a2498ad04cdb3 | |
parent | 17557f4a012a58fb4a20f890a07abc314e5c5abd (diff) |
Implement trace2 manager taking care of trace2 integration
This commit implement a manager object to manage trace2 integration to a
git command. This manager allows hook registration. Before the command
starts, it checks if any hook is interested in trace2 data. If any of
them does, it opens a temporary tempfile, injects the corresponding
environment variables to the command environment variable set. After the
command executes, it collects the result and parses it to the tree.
Finally, it dispatches activated hooks' handlers.
-rw-r--r-- | internal/git/trace2/hook.go | 12 | ||||
-rw-r--r-- | internal/git/trace2/manager.go | 113 | ||||
-rw-r--r-- | internal/git/trace2/manager_test.go | 222 |
3 files changed, 347 insertions, 0 deletions
diff --git a/internal/git/trace2/hook.go b/internal/git/trace2/hook.go new file mode 100644 index 000000000..4ba0a69bf --- /dev/null +++ b/internal/git/trace2/hook.go @@ -0,0 +1,12 @@ +package trace2 + +import "context" + +// Hook is the interface for Trace2 hooks +type Hook interface { + // Name returns the name of the hook + Name() string + // Handle is handler function that a hook registers with the manager. After trace tree is + // built, the manager dispatches handlers in order with the root trace of the tree. + Handle(context.Context, *Trace) error +} diff --git a/internal/git/trace2/manager.go b/internal/git/trace2/manager.go new file mode 100644 index 000000000..229a71dcd --- /dev/null +++ b/internal/git/trace2/manager.go @@ -0,0 +1,113 @@ +package trace2 + +import ( + "context" + "fmt" + "os" +) + +// Manager is responsible for enabling Trace2 for a Git command. It manages the list of hooks who +// are interested in trace2 data. Before the command starts, the manager opens a tempfile. It +// injects the path to this file and some other conventional environment variables to the ENV list +// of the command by calling Inject. After the command exits, the caller is expected to call Finish. +// Finally, the transformed trace2 tree is passed into handlers of registered hooks. +type Manager struct { + sid string + hooks []Hook + fd *os.File + err error +} + +// NewManager returns a Manager object that manages the registered hooks +func NewManager(sid string, hooks []Hook) (*Manager, error) { + if len(hooks) == 0 { + return nil, fmt.Errorf("input hooks are empty") + } + return &Manager{sid: sid, hooks: hooks}, nil +} + +// HookNames return names of hooks +func (m *Manager) HookNames() []string { + var names []string + for _, hook := range m.hooks { + names = append(names, hook.Name()) + } + 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 { + fd, err := os.CreateTemp("", "gitaly-trace2") + if err != nil { + m.err = fmt.Errorf("trace2 create tempfile: %w", err) + return env + } + m.fd = fd + + env = append( + env, + // GIT_TRACE2_EVENT is the key ENV variable. It enables git to dump event format + // target as JSON-based format. When the path to the file is supplied, it *appends* + // the events to the appointed file. Child processes inherits the same ENV set. + // Thus, their events are dumped to the same file. This file is cleaned up when + // calling Finish(). + fmt.Sprintf("GIT_TRACE2_EVENT=%s", fd.Name()), + // GIT_TRACE2_PARENT_SID is the unique identifier of a process. As PID number is + // re-used, Git uses SID number to identify the owner of an event + fmt.Sprintf("GIT_TRACE2_PARENT_SID=%s", m.sid), + // GIT_TRACE2_BRIEF strips redundant information, such as time, file, line, etc. + // This variable makes the output data compact enough to use on production. One + // notable stripped field is time. The time information is available in some key + // events. Subsequent events must infer their time from relative float time diff. + "GIT_TRACE2_BRIEF=true", + // Apart from the above variables, there are some non-documented interesting + // variables, such as GIT_TRACE2_CONFIG_PARAMS or GIT_TRACE2_ENV_VARS. We can + // consider adding them in the future. The full list can be found at: + // https://github.com/git/git/blob/master/trace2.h + ) + return env +} + +// 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 + } + + defer func() { + if err := m.fd.Close(); err != nil { + if m.err == nil { + m.err = fmt.Errorf("trace2: close tempfile: %w", err) + } + // Even if the manager fails to close the tempfile, it should fallthrough to + // remove it from the file system + } + if err := os.Remove(m.fd.Name()); err != nil { + if m.err == nil { + m.err = fmt.Errorf("trace2: remove tempfile: %w", err) + } + } + }() + + trace, err := Parse(ctx, m.fd) + if err != nil { + m.err = fmt.Errorf("trace2: parsing events: %w", err) + return + } + if trace == nil { + m.err = fmt.Errorf("trace2: no events to handle") + return + } + 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 + } + } +} diff --git a/internal/git/trace2/manager_test.go b/internal/git/trace2/manager_test.go new file mode 100644 index 000000000..bdbfd25b0 --- /dev/null +++ b/internal/git/trace2/manager_test.go @@ -0,0 +1,222 @@ +package trace2 + +import ( + "context" + "fmt" + "os" + "regexp" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" +) + +type dummyHook struct { + name string + handler func(context.Context, *Trace) error +} + +func (h *dummyHook) Name() string { + return h.name +} + +func (h *dummyHook) Handle(ctx context.Context, trace *Trace) error { + return h.handler(ctx, trace) +} + +func TestManager(t *testing.T) { + t.Parallel() + + expectedTrace := strings.TrimSpace(` +| | main | root +| | main | .version +| | main | .start +| | main | .def_repo +| | main | .pack-objects:enumerate-objects +| | main | ..progress:Enumerating objects +| | main | .pack-objects:prepare-pack +| | main | ..progress:Counting objects +| | main | .pack-objects:write-pack-file +| | main | ..progress:Writing objects +| | main | ..data:pack-objects:write_pack_file/wrote +| | main | .data:fsync:fsync/writeout-only + +`) + + events := testhelper.MustReadFile(t, "testdata/git-pack-objects.event") + for _, tc := range []struct { + desc string + setup func() (bool, []Hook, func(*testing.T, *Manager)) + expectedErr error + }{ + { + desc: "empty hooks", + setup: func() (bool, []Hook, func(*testing.T, *Manager)) { + return false, nil, nil + }, + expectedErr: fmt.Errorf("input hooks are empty"), + }, + { + desc: "one hook", + setup: func() (bool, []Hook, func(*testing.T, *Manager)) { + hook := dummyHook{ + name: "dummy", + handler: func(ctx context.Context, trace *Trace) error { + require.Equal(t, expectedTrace, trace.Inspect(false)) + return nil + }, + } + return true, []Hook{&hook}, nil + }, + }, + { + desc: "multiple hooks", + setup: func() (bool, []Hook, func(*testing.T, *Manager)) { + var dispatched []string + + hook1 := dummyHook{ + name: "dummy1", + handler: func(ctx context.Context, trace *Trace) error { + dispatched = append(dispatched, "dummy1") + require.Equal(t, expectedTrace, trace.Inspect(false)) + return nil + }, + } + hook2 := dummyHook{ + name: "dummy2", + handler: func(ctx context.Context, trace *Trace) error { + dispatched = append(dispatched, "dummy2") + require.Equal(t, expectedTrace, trace.Inspect(false)) + return nil + }, + } + return true, []Hook{&hook1, &hook2}, func(t *testing.T, manager *Manager) { + require.Equal(t, []string{"dummy1", "dummy2"}, dispatched) + } + }, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + activated, hooks, assert := tc.setup() + + manager, err := NewManager("1234", hooks) + if tc.expectedErr != nil { + require.Equal(t, tc.expectedErr, err) + return + } + + require.NoError(t, err) + + envs := []string{"CORRELATION_ID=1234"} + injectedEnvs := manager.Inject(envs) + + if activated { + require.Equal(t, "CORRELATION_ID=1234", injectedEnvs[0]) + require.Regexp(t, regexp.MustCompile("^GIT_TRACE2_EVENT=.*$"), injectedEnvs[1]) + require.Equal(t, "GIT_TRACE2_PARENT_SID=1234", injectedEnvs[2]) + require.Equal(t, "GIT_TRACE2_BRIEF=true", injectedEnvs[3]) + + require.FileExists(t, manager.fd.Name()) + require.NoError(t, os.WriteFile(manager.fd.Name(), events, os.ModeAppend)) + } else { + require.Equal(t, envs, injectedEnvs) + require.Nil(t, manager.fd) + } + + manager.Finish(testhelper.Context(t)) + require.NoError(t, manager.Error()) + if activated { + require.NoFileExists(t, manager.fd.Name()) + } + if assert != nil { + assert(t, manager) + } + }) + } +} + +func TestManager_tempfileFailures(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + desc string + setup func(*testing.T, *Manager) + expectedError *regexp.Regexp + }{ + { + desc: "invalid events", + setup: func(t *testing.T, manager *Manager) { + require.NoError(t, os.WriteFile(manager.fd.Name(), []byte("something invalid"), os.ModeAppend)) + }, + expectedError: regexp.MustCompile("trace2: parsing events: reading event: decoding event: invalid character 's' looking for beginning of value"), + }, + { + desc: "tempfile closed", + setup: func(t *testing.T, manager *Manager) { + require.NoError(t, manager.fd.Close()) + }, + expectedError: regexp.MustCompile("trace2: parsing events: reading event: decoding event:.*file already closed$"), + }, + { + desc: "tempfile removed", + setup: func(t *testing.T, manager *Manager) { + require.NoError(t, os.Remove(manager.fd.Name())) + }, + expectedError: regexp.MustCompile("trace2: no events to handle$"), + }, + } { + t.Run(tc.desc, func(t *testing.T) { + hook := dummyHook{ + name: "dummy", + handler: func(ctx context.Context, trace *Trace) error { + require.Fail(t, "must not trigger handler if event file has troubles") + return nil + }, + } + + manager, err := NewManager("1234", []Hook{&hook}) + require.NoError(t, err) + + _ = manager.Inject([]string{}) + + tc.setup(t, manager) + manager.Finish(testhelper.Context(t)) + + require.Regexp(t, tc.expectedError, manager.Error().Error()) + require.NoFileExists(t, manager.fd.Name()) + }) + } +} + +func TestManager_handlerFailures(t *testing.T) { + t.Parallel() + + hook1 := dummyHook{ + name: "dummy1", + handler: func(ctx context.Context, trace *Trace) error { return nil }, + } + hook2 := dummyHook{ + name: "dummy2", + handler: func(ctx context.Context, trace *Trace) error { return fmt.Errorf("something goes wrong") }, + } + hook3 := dummyHook{ + name: "dummy3", + handler: func(ctx context.Context, trace *Trace) error { + require.Fail(t, "should not trigger the next hook if the prior one fails") + return nil + }, + } + + manager, err := NewManager("1234", []Hook{&hook1, &hook2, &hook3}) + require.NoError(t, err) + + _ = manager.Inject([]string{}) + + 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)) + + require.Equal(t, `trace2: executing "dummy2" handler: something goes wrong`, manager.Error().Error()) + require.NoFileExists(t, manager.fd.Name()) +} |