Logging Part 3: child process tracing #30
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The 'time' field of the GIT_TRACE2_EVENT json logs should be UTC, per the API specification [1]. Additionally, the timestamp "seconds" field should be reported out to 6 decimal places, with trailing zeroes. Update the internal logger's time encoder to match these requirements. [1] https://git-scm.com/docs/api-trace2#Documentation/technical/api-trace2.txt-codetimelttimegtcode Signed-off-by: Victoria Dye <[email protected]>
Rather than log durations as 'Float64' of the duration's '.Seconds()', use 'zap.Duration()' directly and configure the logger to report durations in units of seconds. Signed-off-by: Victoria Dye <[email protected]>
Move 'command.go' (containing the 'CommandExecutor' interface) into a new package 'cmd'. This helps set up for future enhancements to the interface, including the addition of supplemental types (like 'cmd.Setting', used to configure the behavior of the 'Run()' function). Signed-off-by: Victoria Dye <[email protected]>
Continue the work started in e8713a4 (git-bundle-server: propagate 'context.Context' through call stack, 2023-02-21) by adding 'ctx context.Context' to the 'Run()' method in the 'CommandExecutor'. When logging is added to these methods in later commits, the context will be used to call the appropriate log functions. Signed-off-by: Victoria Dye <[email protected]>
Initialize the 'CommandExecutor' with an instance of 'log.TraceLogger', consistent with other structures (e.g. 'core.RepoProvider'). This will be used to log the setup and run operations of the executor in later commits. Signed-off-by: Victoria Dye <[email protected]>
Create a 'KeyValue' type, identical to 'testhelpers.Pair' other than its
field names ('Key'/'Value' vs. 'First'/'Second'). See the comment in the
newly-created 'utils/types.go' for more information about why a new type was
created, rather than moving 'testhelpers.Pair' into 'utils'.
This 'KeyValue' type will be used in future commits as a way of passing
arbitrary configuration options to functions.
Signed-off-by: Victoria Dye <[email protected]>
Change the function signature of 'Run()' to accept command arguments in a '[]string' (rather than variadic args) and add a set of 'cmd.Setting' variadic args to optionally configure the command execution. This configurability will be necessary for cutting over things like 'git.go' (which, for example, sometimes specifies a custom stdin buffer) to use the 'CommandExecutor'. To maintain the old function signatures, also add convenience wrappers 'RunQuiet()' and 'RunStdout()'. Signed-off-by: Victoria Dye <[email protected]>
Add 'ChildProcess()' to 'log.TraceLogger()', with a corresponding implementation in the Trace2 logger. 'ChildProcess()' works somewhat similarly to 'Region()', writing a "child_start" log during its main execution and returning two functions to write "child_ready" and "child_exit" logs. The main difference from 'Region()', though, is that the returned functions are meant to be invoked directly at the appropriate times in the process execution (rather than deferred). Add the child process logging to 'CommandExecutor.Run()'. Signed-off-by: Victoria Dye <[email protected]>
derrickstolee
approved these changes
Mar 3, 2023
Comment on lines
+42
to
+45
| case stdinKey: | ||
| cmd.Stdin = setting.Value.(io.Reader) | ||
| case stdoutKey: | ||
| cmd.Stdout = setting.Value.(io.Writer) |
There was a problem hiding this comment.
I had to double-check my Go book to realize we don't need break statements here.
internal/common/filesystem.go
Outdated
| return fmt.Errorf("error creating parent directories: %w", err) | ||
| } | ||
|
|
||
| err = os.WriteFile(filename, content, 0o644) | ||
| err = os.WriteFile(filename, content, 0o600) |
There was a problem hiding this comment.
Would it be helpful to extract 0o600 as a constant somewhere?
There was a problem hiding this comment.
Sure! Although I don't plan to replace the the hardcoded values in repo.go/bundles.go with the constant since my goal is to eventually replace their os.WriteFile/os.OpenFile/etc. usage with use of common.FileSystem.
cmd/utils/cron.go
Outdated
Comment on lines
26
to
30
| return &cronHelper{ | ||
| fileSystem: fs, | ||
| scheduler: s, | ||
| } |
There was a problem hiding this comment.
I think you forgot the logger here.
Currently, the only usage of 'WriteFile()' is in the daemon providers, used to write out their respective daemon configurations. The permissions '644' were selected somewhat arbitrarily (corresponding to Git's "non-executable" file mode). However, all other file writing in the repository uses the permissions '600' (removing read permissions for group & all). The daemon configuration files do not actually need anything more permissive than '600', so change 'WriteFile()'s permissions accordingly. Then, rather than simply modify the hardcoded value, create a const 'DefaultFilePermissions' to store the value, and do the same for 0o755 as 'DefaultDirPermissions'. In addition to internal consistency, this sets us up to more easily use 'common.FileSystem' for file writing across the repository. Signed-off-by: Victoria Dye <[email protected]>
Add a function 'GetLocalExecutable()' to 'FileSystem'. This function, given
the identifier of one of the executables installed with this package
('git-bundle-server' or 'git-bundle-web-server'), will look for the file in
the same directory as the current executing program, and return its absolute
path.
This function is then used to replace the custom logic in
'getDaemonConfig()', which locates the 'git-bundle-web-server' executable to
run in a daemon process. This is a functional change; previously,
'getDaemonConfig()' would first lookup the executable with 'exec.LookPath()'
before falling back on the local directory. Removing the 'exec.LookPath()'
seems like the right thing to do here - if we're running a local version of
'git-bundle-server', we'd want to find the executable corresponding to that
local build.
Signed-off-by: Victoria Dye <[email protected]>
Refactor 'internal/cron.go' into a 'CronScheduler' interface, and
'cmd/git-bundle-server/cron.go' (moved to 'cmd/utils/cron.go') into a
'CronHelper' interface. The former converts standalone functions previously
in the file into struct methods, while also moving much of the job
addition/update logic from 'cmd/git-bundle-server/cron.go' into a new
'AddJob()'. The 'CronHelper' has one method - 'SetCronSchedule()' - which
calls 'AddJob()' to set the daily 'update-all' schedule (used by the 'init'
and 'start' commands).
Both of these structs utilize the existing helper structures ('FileSystem',
'CommandExecutor', and 'UserProvider') to handle all system interaction. As
a result, they can both be unit tested in a future update.
Signed-off-by: Victoria Dye <[email protected]>
Convert the standalone functions in 'git.go' into a 'GitHelper' interface. Like 'CronScheduler' & 'CronHelper', 'GitHelper's use of 'CommandExecutor' sets it up to be unit testable in the future. Signed-off-by: Victoria Dye <[email protected]>
vdye
force-pushed
the
vdye/child-process-tracing
branch
from
c38d505 to
a609c0b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #22.
Previous PRs: #28, #29
Summary
This week in the
neverendingsaga of logging ingit-bundle-serverintroduces child process tracing (specifically,child_start,child_ready, andchild_exittrace2 events)! There's a lot in this PR that's not that, though, so to summarize:time,t_rel, andt_absfields.CommandExecutorto both use alog.TraceLogger(which it will need in commit 8) and make it flexible enough to replace standalone usage ofcommand.Exec().log.TraceLogger&log.Trace2(theChildProcess()method)common.FileSystemto handle the file writing & other usage needed by bothcron.gos &git.gocron.gos andgit.goto move their functions into structs, replacing custom file writing & command execution with use ofcommon.FileSystemandcmd.CommandExecutor(the latter of which includes the newly-added Trace2 child process logging).