Re: [PATCH v3 00/10] trace2: load trace2 settings from system config

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Apr 12, 2019, 13:47 UTC
Message-ID: <b4236c9b-8fc2-3704-dfcd-ae71962ac6eb@jeffhostetler.com>
In-Reply-To: <xmqqv9zkymzd.fsf@gitster-ct.c.googlers.com>

On 4/11/2019 10:29 PM, Junio C Hamano wrote:

> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>

[...]

Show 31 quoted lines

>>       @@ -512,19 +454,28 @@
>>        + */
>>        +/* clang-format off */
>>
>>       ++	[TR2_SYSENV_CFG_PARAM]     = { "GIT_TR2_CONFIG_PARAMS",
>>       ++				       "trace2.configparams" },
>>       ++
>>       ++	[TR2_SYSENV_DST_DEBUG]     = { "GIT_TR2_DST_DEBUG",
>>       ++				       "trace2.destinationdebug" },
>>       ++
>>       ++	[TR2_SYSENV_NORMAL]        = { "GIT_TR2",
>>       ++				       "trace2.normaltarget" },
>>       ++	[TR2_SYSENV_NORMAL_BRIEF]  = { "GIT_TR2_BRIEF",
>>       ++				       "trace2.normalbrief" },
>>       ++
>>       ++	[TR2_SYSENV_EVENT]         = { "GIT_TR2_EVENT",
>>       ++				       "trace2.eventtarget" },
>>       ++	[TR2_SYSENV_EVENT_BRIEF]   = { "GIT_TR2_EVENT_BRIEF",
>>       ++				       "trace2.eventbrief" },
>>       ++	[TR2_SYSENV_EVENT_NESTING] = { "GIT_TR2_EVENT_NESTING",
>>       ++				       "trace2.eventnesting" },
>>       ++
>>       ++	[TR2_SYSENV_PERF]          = { "GIT_TR2_PERF",
>>       ++				       "trace2.perftarget" },
>>       ++	[TR2_SYSENV_PERF_BRIEF]    = { "GIT_TR2_PERF_BRIEF",
>>       ++				       "trace2.perfbrief" },
> 
> 
> With use of designated initializers, the table got a lot cleaner to
> read.  Is the above "format off" still needed (I am a bit curious
> how clang-format wants these entries to look like)?

clang-format suggests getting rid of the extra whitespace on the lines, so we lose all of the column alignment. Then it wants to line-wrap some but not all of the lines. So it is a bit of a mess to look at.

Show 19 quoted lines

> 
>>       ++	if (pid > 999999)
>>       ++		strbuf_addf(&tr2sid_buf, "W%06d", (int)(pid % 1000000));
>>       ++	else
>>       ++		strbuf_addf(&tr2sid_buf, "P%06d", (int)pid);
> 
> I do not think it matters too much, but this is kind-of curious.
> 
> How would the users of the log utilize the distinction between W and
> P?  Do they discard the ones with W when they care about the exact
> process that left the trace entries, or something?  If it's not a
> plausibly useful use pattern (and I do not think it is), I wonder if
> we want to go with only W (i.e. truncated to the lower N digits)
> entries, if you are shooting for a fixed-width output from this
> function.  If you want less chance of collisions, you obviously
> could use hexadecimal to gain back a few more bits.
> 
> After all, if the application does care the PID, that could be in
> the log data itself (i.e. an "start" event can say "my pid is blah").

Right. Ævar suggested adding the full or wrapped PID so that the SID would be a fixed length. I stuck with decimal rather than hex because it's easier to match up a running command with '/usr/bin/ps' output, but that's no big deal either way. It might be simpler to just %08lx it and be done with it.

Jeff