git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, peff@peff.net, jrnieder@gmail.com,
	steadmon@google.com, avarab@gmail.com
Subject: Re: [PATCH v3 00/10] trace2: load trace2 settings from system config
Date: Fri, 12 Apr 2019 11:29:58 +0900
Message-ID: <xmqqv9zkymzd.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <pull.169.v3.git.gitgitgadget@gmail.com>

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

> Here is version 3.
> [] It incorporates Ævar's suggestions WRT the format and uniqueness of the
> SID. [] It now reads both system and global config for trace2 settings and
> handles includes as Jonathan suggested.

Following the ISO more closely with Ts and Zs in the format looks
like a good idea (i.e. it gives a more familiar-looking result).  I
have no string opinions on the per-user config or inclusion, but I
think it probably is an essential ingredient to give an opt-out
escape hatch to make this appear less big-brotherly [*1*].

	Side note *1*: and hence less scary.  An otherwise useful
	mechanism can have an appearance that it can also be misused
	for bad purposes, and at that point, to those with fear, it
	does not matter how useful an application the mechanism has.

	I think "by default you are opting in due to your belonging
	to this organization in the first place (hence we give you
	/etc/gitconfig that lets us collect your usage patterns) but
	you could easily opt-out by overriding in $HOME/.gitconfig"
	strikes a good balance.

> I added a read_very_early_config() function that is similar to 
> read_early_config()but omits repo local, worktree, and -c command line
> settings. This felt like a little bit of a hack, but it made the intent
> clear.

I am not yet judging if the "very early config" itself is a good
thing to have (and if it is good, then it is not a "hack" ;-)), but
I very much agree that it is a very good change to have a helper
that makes the intent clear.

>      -+# Now test using system config by using a mocked up config file
>      -+# rather than inheriting "/etc/gitconfig".  Here we do not use
>      -+# GIT_TR2* environment variables.
>      -+
>       +unset GIT_TR2_BRIEF


This does not have to be sane_unset, as we are not aiming for making
our tests "set -e" clean.  But in tXXXX-*.sh scripts, it may not be
a bad idea to stick to sane_unset regardless, as people tend to cut
and paste without thinking enough.

>      @@ -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)?

>      ++	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").

Thanks.  I'll wait until learning what others think.

  parent reply index

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-28 13:30 [PATCH 0/4] " Jeff Hostetler via GitGitGadget
2019-03-28 13:30 ` [PATCH 1/4] trace2: refactor setting process starting time Jeff Hostetler via GitGitGadget
2019-03-28 13:31 ` [PATCH 2/4] trace2: add absolute elapsed time to start event Jeff Hostetler via GitGitGadget
2019-03-28 13:31 ` [PATCH 3/4] trace2: find exec-dir before trace2 initialization Jeff Hostetler via GitGitGadget
2019-03-28 13:31 ` [PATCH 4/4] trace2: use system config for default trace2 settings Jeff Hostetler via GitGitGadget
2019-03-28 14:36   ` Ævar Arnfjörð Bjarmason
2019-03-28 18:50     ` Jeff Hostetler
2019-03-28 21:28   ` Josh Steadmon
2019-03-29 17:04 ` [PATCH v2 0/7] trace2: load trace2 settings from system config Jeff Hostetler via GitGitGadget
2019-03-29 17:04   ` [PATCH v2 1/7] trace2: refactor setting process starting time Jeff Hostetler via GitGitGadget
2019-03-29 17:04   ` [PATCH v2 2/7] trace2: add absolute elapsed time to start event Jeff Hostetler via GitGitGadget
2019-03-29 17:04   ` [PATCH v2 3/7] trace2: find exec-dir before trace2 initialization Jeff Hostetler via GitGitGadget
2019-03-29 17:04   ` [PATCH v2 4/7] trace2: use system config for default trace2 settings Jeff Hostetler via GitGitGadget
2019-04-01 21:00     ` Josh Steadmon
2019-04-01 21:06       ` Jeff Hostetler
2019-04-03  0:01       ` Jonathan Nieder
2019-04-03  0:00     ` Jonathan Nieder
2019-04-09 15:58       ` Jeff Hostetler
2019-03-29 17:04   ` [PATCH v2 5/7] trace2: report peak memory usage of the process Jeff Hostetler via GitGitGadget
2019-03-29 22:16     ` Ævar Arnfjörð Bjarmason
2019-04-01 21:05       ` Jeff Hostetler
2019-03-29 17:04   ` [PATCH v2 6/7] trace2: clarify UTC datetime formatting Jeff Hostetler via GitGitGadget
2019-03-29 17:04   ` [PATCH v2 7/7] trace2: make SIDs more unique Jeff Hostetler via GitGitGadget
2019-03-29 22:12     ` Ævar Arnfjörð Bjarmason
2019-04-01 21:16       ` Jeff Hostetler
2019-04-01 21:02   ` [PATCH v2 0/7] trace2: load trace2 settings from system config Josh Steadmon
2019-04-11 15:18   ` [PATCH v3 00/10] " Jeff Hostetler via GitGitGadget
2019-04-11 15:18     ` [PATCH v3 01/10] config: initialize opts structure in repo_read_config() Jeff Hostetler via GitGitGadget
2019-04-12  3:52       ` Jonathan Nieder
2019-04-15 14:34         ` Johannes Schindelin
2019-04-11 15:18     ` [PATCH v3 02/10] trace2: refactor setting process starting time Jeff Hostetler via GitGitGadget
2019-04-11 15:18     ` [PATCH v3 03/10] trace2: add absolute elapsed time to start event Jeff Hostetler via GitGitGadget
2019-04-11 15:18     ` [PATCH v3 04/10] trace2: find exec-dir before trace2 initialization Jeff Hostetler via GitGitGadget
2019-04-11 15:18     ` [PATCH v3 05/10] config: add read_very_early_config() Jeff Hostetler via GitGitGadget
2019-04-11 15:18     ` [PATCH v3 06/10] trace2: use system/global config for default trace2 settings Jeff Hostetler via GitGitGadget
2019-04-11 15:18     ` [PATCH v3 07/10] trace2: report peak memory usage of the process Jeff Hostetler via GitGitGadget
2019-04-11 15:18     ` [PATCH v3 08/10] trace2: clarify UTC datetime formatting Jeff Hostetler via GitGitGadget
2019-04-11 15:18     ` [PATCH v3 10/10] trace2: update docs to describe system/global config settings Jeff Hostetler via GitGitGadget
2019-04-11 15:18     ` [PATCH v3 09/10] trace2: make SIDs more unique Jeff Hostetler via GitGitGadget
2019-04-12  2:29     ` Junio C Hamano [this message]
2019-04-12 13:47       ` [PATCH v3 00/10] trace2: load trace2 settings from system config Jeff Hostetler
2019-04-15 20:39     ` [PATCH v4 " Jeff Hostetler via GitGitGadget
2019-04-15 20:39       ` [PATCH v4 01/10] config: initialize opts structure in repo_read_config() Jeff Hostetler via GitGitGadget
2019-04-15 20:39       ` [PATCH v4 02/10] trace2: refactor setting process starting time Jeff Hostetler via GitGitGadget
2019-04-15 20:39       ` [PATCH v4 03/10] trace2: add absolute elapsed time to start event Jeff Hostetler via GitGitGadget
2019-04-15 20:39       ` [PATCH v4 04/10] trace2: find exec-dir before trace2 initialization Jeff Hostetler via GitGitGadget
2019-04-15 20:39       ` [PATCH v4 05/10] config: add read_very_early_config() Jeff Hostetler via GitGitGadget
2019-04-15 20:39       ` [PATCH v4 06/10] trace2: use system/global config for default trace2 settings Jeff Hostetler via GitGitGadget
2019-04-27 13:43         ` SZEDER Gábor
2019-04-29 19:03           ` Jeff Hostetler
2019-04-15 20:39       ` [PATCH v4 08/10] trace2: clarify UTC datetime formatting Jeff Hostetler via GitGitGadget
2019-04-15 20:39       ` [PATCH v4 07/10] trace2: report peak memory usage of the process Jeff Hostetler via GitGitGadget
2019-04-15 20:39       ` [PATCH v4 09/10] trace2: make SIDs more unique Jeff Hostetler via GitGitGadget
2019-04-15 20:39       ` [PATCH v4 10/10] trace2: update docs to describe system/global config settings Jeff Hostetler via GitGitGadget
2019-04-29 20:14       ` [PATCH v5 00/11] trace2: load trace2 settings from system config Jeff Hostetler via GitGitGadget
2019-04-29 20:14         ` [PATCH v5 02/11] trace2: refactor setting process starting time Jeff Hostetler via GitGitGadget
2019-04-29 20:14         ` [PATCH v5 01/11] config: initialize opts structure in repo_read_config() Jeff Hostetler via GitGitGadget
2019-04-29 20:14         ` [PATCH v5 03/11] trace2: add absolute elapsed time to start event Jeff Hostetler via GitGitGadget
2019-04-29 20:14         ` [PATCH v5 04/11] trace2: find exec-dir before trace2 initialization Jeff Hostetler via GitGitGadget
2019-04-29 20:14         ` [PATCH v5 05/11] config: add read_very_early_config() Jeff Hostetler via GitGitGadget
2019-04-29 20:14         ` [PATCH v5 06/11] trace2: use system/global config for default trace2 settings Jeff Hostetler via GitGitGadget
2019-04-29 20:14         ` [PATCH v5 07/11] trace2: report peak memory usage of the process Jeff Hostetler via GitGitGadget
2019-04-29 20:14         ` [PATCH v5 09/11] trace2: make SIDs more unique Jeff Hostetler via GitGitGadget
2019-04-29 20:14         ` [PATCH v5 08/11] trace2: clarify UTC datetime formatting Jeff Hostetler via GitGitGadget
2019-04-29 20:14         ` [PATCH v5 10/11] trace2: update docs to describe system/global config settings Jeff Hostetler via GitGitGadget
2019-04-29 20:14         ` [PATCH v5 11/11] trace2: fixup access problem on /etc/gitconfig in read_very_early_config Jeff Hostetler via GitGitGadget
2019-04-29 20:21         ` [PATCH v5 00/11] trace2: load trace2 settings from system config Jeff Hostetler
2019-05-07  1:18           ` Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqv9zkymzd.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    --cc=steadmon@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox