git-ecosystem / git-bundle-server

A web server & management CLI to host Git bundles for use with Git's "bundle URIs" feature
Other
43 stars 20 forks source link

Logging Part 1: initial `trace2` events (`start`, `cmd_name`, `error`, `exit`, `atexit`) #28

Closed vdye closed 1 year ago

vdye commented 1 year ago

Part of #22.

Summary

This series creates a (relatively) simple framework for logging events in the git-bundle-server and git-bundle-web-server applications. As noted in #22, I went with trace2 JSON as the log format.

In terms of this pull request, it's roughly broken into "set up to be able to do logging" (patches 1-4) and "create TraceLogger and its Trace2 implementation" (patches 5-10).

Testing

I tried out a bunch of scenarios - abrupt exits due to bad arguments, commands run successfully to completion, errors handled and sent back up the call stack - as well as console logging (with GIT_TRACE2_EVENT=1) and file logging (with GIT_TRACE2_EVENT=<existing dir> and GIT_TRACE2_EVENT=<new file, possibly missing parent dir>).

Future work

There's still a lot to do to get a full set of trace2 logging working, some of which I've started (or at least planned out):

CC: @jeffhostetler

vdye commented 1 year ago

Quick heads-up: while doing some testing, I noticed that the panic() catch was suppressing the error & stack trace. It's a pretty small change, can be found here.

derrickstolee commented 1 year ago

I tried out a bunch of scenarios - abrupt exits due to bad arguments, commands run successfully to completion, errors handled and sent back up the call stack - as well as console logging (with GIT_TRACE2_EVENT=1) and file logging (with GIT_TRACE2_EVENT=<existing dir> and GIT_TRACE2_EVENT=<new file, possibly missing parent dir>).

I think Git requires GIT_TRACE2_EVENT to specify a fully-qualified path in order to write to a file. At least, I keep needing to write GIT_TRACE2_EVENT="$(pwd)/trace" in the test suite instead of just =trace. Having that extra ability here seems fine, though.

jeffhostetler commented 1 year ago

BTW, I have client-side code for talking to a unix socket in my git-telemetry-service-tool https://github.com/github/git-trace2-otel/blob/ccc408440bc03ee0465ecccd7eb7ff875436393d/src/git-telemetry-service-tool/send_other.go#L14

It's just a net.Dial() to set up. And then use the net.Conn.Write() method to send data. My code just takes the socket pathname (without the af_unix: prefix stuff that I had to add to core Git to disambiguate).

Also, you can use the git-telemetry-service-tool to send a txt file containing JSON from a command to the service if you want to see what the data looks like on the other side.

vdye commented 1 year ago

I think Git requires GIT_TRACE2_EVENT to specify a fully-qualified path in order to write to a file. At least, I keep needing to write GIT_TRACE2_EVENT="$(pwd)/trace" in the test suite instead of just =trace. Having that extra ability here seems fine, though.

You're right, the implementation as it is now allows relative paths, while Git doesn't. I'll make sure to note it in the documentation in Part 3!

jeffhostetler commented 1 year ago

WRT absolute paths, yes, Trace2 requires absolute paths within Git because relative paths are ambiguous. That is, there's the context of where you type the git config to set it, where you hack your ~/.gitconfig to manually set it, vs the PWD of where you issue Git commands that generate telemetry. And I couldn't hack into the config code to change a value from a relative to absolute path, so I had to force an absolute path here.