Logging Part 2: region tracing #29
Conversation
Change the return types from pointers to the inheriting type (e.g.
'userProvider') to the interface type ('UserProvider'). This is more
consistent with the rest of the codebase, and more readily provides compiler
errors when the type does not implement the interface.
Signed-off-by: Victoria Dye <[email protected]>
Add a 'DependencyContainer' type, which contains methods to register and lazily retieve singleton dependencies. This is implemented in anticipation of a substantial increase in the complexity of the dependency hierarchy. Its primary benefit is avoiding the need to initialize an entire hierarchy of dependent structs "manually", hopefully making for easier to read (and modify) code. Note, though, that this design carries some risk: - errors due to unregistered components will only manifest at runtime. - we might see potentially slower startup due to the use of reflection; that said, in manual testing, I didn't notice a slowdown. - we can end up in a recursive loop of dependency resolution, ultimately leading to a stack overflow. To mitigate that risk, add a test 'TestBuildGitBundleServerContainer', which uses a combination of reflection and (somewhat fragile, but documented in the test) abstract syntax tree parsing to verify that the container is fully-registered. Signed-off-by: Victoria Dye <[email protected]>
Refactor 'repo.go', changing its standalone functions into methods of a 'RepoProvider' interface. This allows for some simplification of helper struct (e.g. FileSystem) accesses, sets up for future unit testing work by making it a mockable interface, and propagates the 'TraceLogger' struct to its functions. In particular, the logger will be used in later patches to set up region tracing for various functions in the file. Signed-off-by: Victoria Dye <[email protected]>
Refactor 'bundles.go', changing its standalone functions into methods of a 'BundleProvider' interface. Like the 'RepoProvider' refactor before it, this makes the functions mockable for future testing and allows for easy use of the 'TraceLogger' into the bundle handling functions. Signed-off-by: Victoria Dye <[email protected]>
Make 'addBundleToList()' (renamed 'addBundle()') and 'getSortedCreationTokens()' (renamed 'sortedCreationTokens()') methods of the 'BundleList' struct rather than standalone functions. Both of these functions are more appropriately represented as modifiers/accessors of the bundle list, rather than an external computation on a bundle list. Signed-off-by: Victoria Dye <[email protected]>
Change references to non-existent logger functions (from an earlier iteration on the Trace2 logger) to those actually in use by Trace2. Signed-off-by: Victoria Dye <[email protected]>
There was a problem hiding this comment.
This seems like a very interesting pattern for getting regions that is less fragile than the enter/leave pattern we need to follow in Git. Thanks for doing careful refactoring to get us to a place where it is easier to add things like this in the future.
One thing that would be nice to do soon is establish the end-to-end testing scenarios so we can see that these regions are being output as expected.
| // Contains lazily generated singletons of stateless struct used throughout | ||
| // the application. Thread-safe. | ||
| type DependencyContainer struct { | ||
| singletonInitializers *sync.Map | ||
| } |
There was a problem hiding this comment.
Interesting mechanism for mapping types to instances.
In reading the commit message, you mention that this could lead to some slight performance degradation when loading the types dynamically instead of directly. I think this is fine, because for it doesn't really matter in either "mode":
- In the foreground CLI, such as
git-bundle-server init, a few milliseconds won't make much difference to the user experience. - In the background maintenance CLI, we spend most of our time in
git fetchandgit bundle create, so this small cost at process startup is not much at all. - In the web server, these are loaded once per process, then the rest of the processing should be fast as the components are already loaded.
| t.logger.Sync() | ||
| } | ||
|
|
||
| func (t *Trace2) Region(ctx context.Context, category string, label string) (context.Context, func()) { |
There was a problem hiding this comment.
I am unable to determine how I should be using this function based on what I see in this commit. I look forward to those uses in the next one.
There was a problem hiding this comment.
I'll also add an example to the commit message. 🙂
| ctx, exitRegion := b.logger.Region(ctx, "http", "serve") | ||
| defer exitRegion() |
There was a problem hiding this comment.
Well, that's pretty neat. I was hoping for something like a using() block from C#, and this does a pretty decent approximation. If we want multiple "stages" of a process to be tracked with regions, then that's a pretty good signal that those stages should be split into methods, anyway.
internal/bundles/bundles.go
Outdated
|
|
||
| // Given a BundleList | ||
| func (b *bundleProvider) WriteBundleList(ctx context.Context, list *BundleList, repo *core.Repository) error { | ||
| _, exitRegion := b.logger.Region(ctx, "bundles", "write_bundle_list") |
There was a problem hiding this comment.
Here, you don't override the ctx variable like you do in other uses. Is there a reason to do one over the other?
There was a problem hiding this comment.
I meant to override the static analysis check that led to this, but it got lost somewhere in my rebasing. It complains that the ctx result from Region() isn't used later in the function, so it should be replaced with _. But, if someone adds anything that uses ctx in the function, it won't be immediately obvious that they're not using the Region()-overridden value. So the better thing to do is probably to override anyway and suppress the warning.
Thanks for pointing that out, I'll fix it & push an update!
Add a 'Region()' to the 'TraceLogger' interface that starts logging a region
for a given context, created an updated context with region metadata (time
of region start, nesting level), and returns a corresponding "end region"
function used for closing out the region. Implement 'Region()' for the
'Trace2' logger.
Example of adding region tracing to a function 'func myFunc()':
---
func myFunc(ctx context.Context, logger log.TraceLogger) {
ctx, endRegion := logger.Region(ctx, "my_file", "my_func")
defer endRegion()
...the rest of myFunc()...
}
---
While 'endRegion()' can be called manually before each possible 'return',
using 'defer' ensures that the region will exit properly from all 'return's
as well as 'panic'.
Signed-off-by: Victoria Dye <[email protected]>
Using the logger in the 'bundleWebServer' struct, use 'TraceLogger.Region()' to log web requests as a region. Note that, unlike the 'git-bundle-server' regions, these regions will have different session IDs than those of the program startup logs. This is intentional, as the regions being added are scoped to the request, and are not "parented" by the overarching web server session. Signed-off-by: Victoria Dye <[email protected]>
Add 'TraceLogger.Region()' invocations to all public methods of the 'RepoProvider' struct. Signed-off-by: Victoria Dye <[email protected]>
Add 'TraceLogger.Region()' invocations to all public methods of the 'BundleProvider' struct, except for 'CreateInitialBundle()' and 'CreateSingletonBundle()' (due to the relative brevity of those functions). Note that static check suppressions are added to two of the 'Region()' calls to allow overriding the 'ctx' despite it not being used later in the function. This helps future-proof against the easy-to-miss bug where a call is added to the function that uses 'ctx', but because 'ctx' is not overridden with the value from 'Region()', the ctx doesn't include the appropriate region information. Signed-off-by: Victoria Dye <[email protected]>
vdye
force-pushed
the
vdye/region-tracing
branch
from
631db20 to
01d39ac
Compare
Part of #22.
Summary
This implements "part 2" of what was described in the initial Trace2 logging pull request (#28), adding region tracing to
repo.go,bundles.go, andbundle-server.go.The branch is broken up as follows:
New*().uber-go/digorgoogle/wire), but none quite fit the use case I wanted (singleton dependency resolution with one instance per type).I ended up writing a custom dependency resolver, as well as a
containers-helper.gofile to contain the dependency registering code. Unfortunately, this approach (while cleaning things up nicely in the rest of the codebase) opens us up to more potential runtime errors. To (at least partially) mitigate that, I added a few tests to verify that the container(s) created incontainers-helper.gowould resolve properly, and that all types requested from the container are registered. The tests are complicated, but I did my best to comment what was going on. Happy to clarify if anyone has any questions though!repo.goandbundles.goto make it easier to add logging to themdeferbehavior, withRegion()starting region tracing and returning a deferrable function that ends the same region.bundle-server.go,repo.go, andbundles.go. I'm sure more can/will be added in the future, but these seemed like a good set to start with.Future work
In addition to the remaining parts mentioned in #28, I ended up doing a lot of refactoring of
repo.goandbundles.goto usecommon.FileSystemandcommon.CommandExecutor. I pulled out of this PR (since it ended up fairly out-of-scope), but I do plan to eventually finish those and add the appropriate unit tests.