Re: [PATCH 01/14] trace2: Documentation/technical/api-trace2.txt
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 23, 2019, 20:51 UTC
- Message-ID
- <xmqqbm47ayhv.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <1a90de9dab0dd836e54fee9e08ab9e2284e1027a.1548192131.git.gitgitgadget@gmail.com>
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +These high-level events are written to one or more Trace2 Targets > +in a target-specific format. Each Trace2 Target defines a different > +purpose-specific view onto the event data stream. In this mannor,
"In this manner"
Show 14 quoted lines
> +a single set of Trace2 API event calls in the Git source can drive > +different types of analysis. > + > ... > +$ cat ~/log.perf > +12:28:42.620675 common-main.c:38 | d0 | main | version | | | | | 2.20.1.155.g426c96fcdb > +12:28:42.621001 common-main.c:39 | d0 | main | start | | | | | git version > +12:28:42.621111 git.c:432 | d0 | main | cmd_verb | | | | | version (version) > +12:28:42.621225 git.c:662 | d0 | main | exit | | 0.001227 | | | code:0 > +12:28:42.621259 trace2/tr2_tgt_perf.c:211 | d0 | main | atexit | | 0.001265 | | | code:0 > +------------ > ... > + > +trace2_def_param(...)
Not limited to this single one, but please either
- omit "..." in parens, unless all of these functions take varargs of unspecified type (which I do not think is the case), or
- write a proper prototype for these functions, explain what the function itself and what the parameters are for.
I'll complain about lack of info around here later X-<.
> +trace2_def_repo(...)
Show 23 quoted lines
> +---------------- > + > +Git Child Process Events:: > + > + These are concerned with the various spawned child processes, > + including sub-git processes and hooks, > ++ > +---------------- > +trace2_child_start(...) > +trace2_child_exit(...) > + > +trace2_exec(...) > +trace2_exec_result(...) > +---------------- > + > +Git Thread Events:: > + > + These are concerned with Git thread usage. > ++ > +---------------- > +trace2_thread_start(...) > +trace2_thread_exit(...) > +----------------
Lack of _wait()/_join() feels a bit curious, but _exit() from what is being waited would suffice as a substitute.
Show 15 quoted lines
> +Initialization::
> +
> + Initialization happens in `main()`. The initialization code
> + determines which, if any, Trace2 Targets should be enabled and
> + emits the `version`, `start`, and `exit` events. It causes an
> + `atexit` function and `signal` handler to be registered that
> + will emit `atexit` and `signal` events.
> ++
> +----------------
> +int main(int argc, const char **argv)
> +{
> + int exit_code;
> +
> + trace2_initialize();
> + trace2_cmd_start(argv);Nobody other than trace2 integration would make a call to this helper, so it may not matter, but sending av alone without ac, even though ac is almost always redundant, feels somewhat unexpected.
Show 12 quoted lines
> +Command Details::
> +
> + After the basics are established, additional process
> + information can be sent to Trace2 as it is discovered, such as
> + the command verb, alias expansion, interesting config
> + settings, the repository worktree, error messages, and etc.
> ++
> +----------------
> +int cmd_checkout(int argc, const char **argv)
> +{
> + // emit a single "def_param" event
> + trace2_def_param("core.editor", "emacs");Without knowing what "def_param event" is, this example is hard to fathom. At this point in the doc, the reader does not even know what "def" stands for. Is this call to define a param called core.editor? Is it reporting that the default value for core.editor is emacs?
> + // emit def_params for any config setting matching a pattern > + // in GIT_TR2_CONFIG_PARAMS. > + trace2_cmd_list_config();
As the reader does not know what def_param is, this is also hard to follow.
> + trace2_cmd_verb("checkout");
> + trace2_cmd_subverb("branch");These are guessable. It probably reports what the codepath is doing.
> + trace2_def_repo(the_repository);
Again, this is not easy to guess. Is it reporting what the default repository is?
> + if (do_something(...))
> + trace2_cmd_error("Path '%s': cannot do something", path);This is guessable, which is good.
Show 9 quoted lines
> +void run_child(...)
> +{
> + int child_exit_code;
> + struct child_process cmd = CHILD_PROCESS_INIT;
> + ...
> +
> + trace2_child_start(&cmd);
> + child_exit_code = ...();
> + trace2_child_exit(&cmd, child_exit_code);Even though child_exit() has not been explained just like def_param(), this is far more guessable. I wonder why it is. The name of the variable passed to it in the example certainly helps, as it is totally unclear where the string constant "emacs" in the earlier example came from (e.g. is it hardcoded in the program? is it merely illustrating "here is how you report the value you have decided to use for 'core.editor' variable"?).
Show 8 quoted lines
> ... > +$ cat ~/log.perf > +d0 | main | version | | | | | 2.20.1.160.g5676107ecd.dirty > +d0 | main | start | | | | | git status > +d0 | main | def_repo | r1 | | | | worktree:/Users/jeffhost/work/gfw > +d0 | main | cmd_verb | | | | | status (status) > +... > +d0 | main | region_enter | r1 | 0.010988 | | status | label:worktrees
It is hard to guess what "d0" and "r1" stand for here.
In an earlier example we also saw an unexplained "d0" there, which I think was OK because the illustration was merely to give the "feel" of the format up there. But now we are giving a bit more detailed explanation, we probably would want to at least briefly mention what each column means.
I'd stop here for now, as I am more interested in reading the code ;-)