From: Jeff Hostetler Date: Fri, 12 Apr 2019 13:47:51 GMT Subject: Re: [PATCH v3 00/10] trace2: load trace2 settings from system config Message-ID: In-Reply-To: On 4/11/2019 10:29 PM, Junio C Hamano wrote: > "Jeff Hostetler via GitGitGadget" writes: > [...] >> @@ -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. > >> ++ 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