trace2: move generation of 'def_param' events into code for 'cmd_name' #1679

jeffhostetler · 2024-02-29T22:54:45Z

Here is version 2 of this series. The only change from V1 is to combine the last
two commits as discussed.

Thanks
Jeff

Some Git commands do not emit def_param events for interesting config and
environment variable settings. Let's fix that.

Builtin commands compiled into git.c have the normal control flow and emit
a cmd_name event and then def_param events for each interesting config and
environment variable. However, some special "query" commands, like --exec-path,
or some forms of alias expansion, emitted a cmd_name but did not emit def_param
events.

Also, special commands git-remote-https is built from remote-curl.c and
git-http-fetch is built from http-fetch.c and do not use the normal set up in git.c.
These emitted a cmd_name but not def_param events.

To minimize the footprint of this commit, move the calls to trace2_cmd_list_config()
and trace2_cmd_list_env_vars() into trace2_cmd_name() so that we always get a
set of def_param events when a cmd_name event is generated.

Users can define local config settings on a repo to classify/name a repo (e.g. "project-foo"
vs "personal") and use the def_param feature to label Trace2 data so that (a third-party)
telemetry service does not collect data on personal repos or so that telemetry from one
work repo is distinguishable from another work repo in database queries.

cc: Josh Steadmon [email protected]
cc: Jeff Hostetler [email protected]

gitgitgadget · 2024-03-01T19:31:23Z

There are issues in commit fcf1027:
t0211: Demonstrate missing 'def_param' events for certain commands
Prefixed commit message must be in lower case

dscho · 2024-03-03T12:01:47Z

@jeffhostetler I opened jeffhostetler#13 to address all those CI failures:

Subject: fixup! t0211: demonstrate missing 'def_param' events for certain commands

diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 30c7a55233a..fe23b8b9d48 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -344,10 +344,10 @@ test_expect_success 'expect def_params for remote-curl and _run_dashed_' '
 
 	test_config_global "cfg.prop.foo" "red" &&
 
-	ENV_PROP_FOO=blue \
+	test_might_fail env \
+		ENV_PROP_FOO=blue \
 		GIT_TRACE2_PERF="$(pwd)/prop.perf" \
-			test_might_fail \
-				git remote-http x y &&
+		git remote-http x y &&
 
 	perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
 
@@ -368,10 +368,10 @@ test_expect_success 'expect def_params for http-fetch and _run_dashed_' '
 
 	test_config_global "cfg.prop.foo" "red" &&
 
-	ENV_PROP_FOO=blue \
+	test_might_fail env \
+		ENV_PROP_FOO=blue \
 		GIT_TRACE2_PERF="$(pwd)/prop.perf" \
-			test_might_fail \
-				git http-fetch --stdin file:/// <<-EOF &&
+		git http-fetch --stdin file:/// <<-EOF &&
 	EOF
 
 	perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&

gitgitgadget · 2024-03-04T14:46:15Z

There are issues in commit 70cb3a7:
fixup! t0211: demonstrate missing 'def_param' events for certain commands
Commit checks stopped - the message is too short
Rebase needed to squash commit
Commit not signed off

Some Git commands fail to emit 'def_param' events for interesting config and environment variable settings. Add unit tests to demonstrate this. Most commands are considered "builtin" and are based upon git.c. These typically do emit 'def_param' events. Exceptions are some of the "query" commands, the "run-dashed" mechanism, and alias handling. Commands built from remote-curl.c (instead of git.c), such as "git-remote-https", do not emit 'def_param' events. Likewise, "git-http-fetch" is built http-fetch.c and does not emit them. Signed-off-by: Jeff Hostetler <[email protected]>

During nested alias expansion it is possible for "trace2_cmd_list_config()" and "trace2_cmd_list_env_vars()" to be called more than once. This causes a full set of 'def_param' events to be emitted each time. Let's avoid that. Add code to those two functions to only emit them once. Signed-off-by: Jeff Hostetler <[email protected]>

jeffhostetler · 2024-03-04T15:39:19Z

/submit

gitgitgadget · 2024-03-04T15:40:20Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1679/jeffhostetler/always-emit-def-param-v1

To fetch this version to local tag pr-1679/jeffhostetler/always-emit-def-param-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1679/jeffhostetler/always-emit-def-param-v1

gitgitgadget · 2024-03-04T22:43:35Z

This branch is now known as jh/trace2-missing-def-param-fix.

gitgitgadget · 2024-03-04T22:43:36Z

This patch series was integrated into seen via git@f991be6.

gitgitgadget · 2024-03-04T22:53:25Z

There was a status update in the "New Topics" section about the branch jh/trace2-missing-def-param-fix on the Git mailing list:

Some trace2 events that lacked def_param have learned to show it,
enriching the output.

Needs review.
source: <[email protected]>

gitgitgadget · 2024-03-05T18:44:16Z

This patch series was integrated into seen via git@9f9a9f0.

gitgitgadget · 2024-03-06T21:49:12Z

git.c

@@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)
 			strvec_pushv(&child.args, (*argv) + 1);


On the Git mailing list, Josh Steadmon wrote (reply to this):

On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote: > From: Jeff Hostetler <[email protected]> > > Now that "trace2_cmd_name()" implicitly calls "trace2_cmd_list_config()" > and "trace2_cmd_list_env_vars()", we don't need to explicitly call them. > > Signed-off-by: Jeff Hostetler <[email protected]> > --- > git.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/git.c b/git.c > index 7068a184b0a..a769d72ab8f 100644 > --- a/git.c > +++ b/git.c > @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv) > strvec_pushv(&child.args, (*argv) + 1); > > trace2_cmd_alias(alias_command, child.args.v); > - trace2_cmd_list_config(); > - trace2_cmd_list_env_vars(); > trace2_cmd_name("_run_shell_alias_"); > > ret = run_command(&child); > @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv) > COPY_ARRAY(new_argv + count, *argv + 1, *argcp); > > trace2_cmd_alias(alias_command, new_argv); > - trace2_cmd_list_config(); > - trace2_cmd_list_env_vars(); > > *argv = new_argv; > *argcp += count - 1; > @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) > > trace_argv_printf(argv, "trace: built-in: git"); > trace2_cmd_name(p->cmd); > - trace2_cmd_list_config(); > - trace2_cmd_list_env_vars(); > > validate_cache_entries(the_repository->index); > status = p->fn(argc, argv, prefix); > -- > gitgitgadget > I'd personally prefer to see this squashed into Patch 3, but I don't feel too strongly about it. Either way, the series LGTM. Reviewed-by: Josh Steadmon <[email protected]>

On the Git mailing list, Junio C Hamano wrote (reply to this):

Josh Steadmon <[email protected]> writes: > On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote: >> From: Jeff Hostetler <[email protected]> >> >> Now that "trace2_cmd_name()" implicitly calls "trace2_cmd_list_config()" >> and "trace2_cmd_list_env_vars()", we don't need to explicitly call them. >> >> Signed-off-by: Jeff Hostetler <[email protected]> >> --- >> git.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/git.c b/git.c >> index 7068a184b0a..a769d72ab8f 100644 >> --- a/git.c >> +++ b/git.c >> @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv) >> strvec_pushv(&child.args, (*argv) + 1); >> >> trace2_cmd_alias(alias_command, child.args.v); >> - trace2_cmd_list_config(); >> - trace2_cmd_list_env_vars(); >> trace2_cmd_name("_run_shell_alias_"); >> >> ret = run_command(&child); >> @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv) >> COPY_ARRAY(new_argv + count, *argv + 1, *argcp); >> >> trace2_cmd_alias(alias_command, new_argv); >> - trace2_cmd_list_config(); >> - trace2_cmd_list_env_vars(); >> >> *argv = new_argv; >> *argcp += count - 1; >> @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) >> >> trace_argv_printf(argv, "trace: built-in: git"); >> trace2_cmd_name(p->cmd); >> - trace2_cmd_list_config(); >> - trace2_cmd_list_env_vars(); >> >> validate_cache_entries(the_repository->index); >> status = p->fn(argc, argv, prefix); >> -- >> gitgitgadget >> > > I'd personally prefer to see this squashed into Patch 3, but I don't > feel too strongly about it. Either way, the series LGTM. > > Reviewed-by: Josh Steadmon <[email protected]> Let's see what JeffH says about this. I agree with you that making some stuff redundant in [Patch 3/4] and fixing the redundancy in this step does feel somewhat roundabout way of doing this. Thanks.

On the Git mailing list, Jeff Hostetler wrote (reply to this):

On 3/6/24 4:57 PM, Junio C Hamano wrote: > Josh Steadmon <[email protected]> writes: > >> On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote: >>> From: Jeff Hostetler <[email protected]> >>> >>> Now that "trace2_cmd_name()" implicitly calls "trace2_cmd_list_config()" >>> and "trace2_cmd_list_env_vars()", we don't need to explicitly call them. >>> >>> Signed-off-by: Jeff Hostetler <[email protected]> >>> --- >>> git.c | 6 ------ >>> 1 file changed, 6 deletions(-) >>> >>> diff --git a/git.c b/git.c >>> index 7068a184b0a..a769d72ab8f 100644 >>> --- a/git.c >>> +++ b/git.c >>> @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv) >>> strvec_pushv(&child.args, (*argv) + 1); >>> >>> trace2_cmd_alias(alias_command, child.args.v); >>> - trace2_cmd_list_config(); >>> - trace2_cmd_list_env_vars(); >>> trace2_cmd_name("_run_shell_alias_"); >>> >>> ret = run_command(&child); >>> @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv) >>> COPY_ARRAY(new_argv + count, *argv + 1, *argcp); >>> >>> trace2_cmd_alias(alias_command, new_argv); >>> - trace2_cmd_list_config(); >>> - trace2_cmd_list_env_vars(); >>> >>> *argv = new_argv; >>> *argcp += count - 1; >>> @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) >>> >>> trace_argv_printf(argv, "trace: built-in: git"); >>> trace2_cmd_name(p->cmd); >>> - trace2_cmd_list_config(); >>> - trace2_cmd_list_env_vars(); >>> >>> validate_cache_entries(the_repository->index); >>> status = p->fn(argc, argv, prefix); >>> -- >>> gitgitgadget >>> >> >> I'd personally prefer to see this squashed into Patch 3, but I don't >> feel too strongly about it. Either way, the series LGTM. >> >> Reviewed-by: Josh Steadmon <[email protected]> > > Let's see what JeffH says about this. I agree with you that making > some stuff redundant in [Patch 3/4] and fixing the redundancy in > this step does feel somewhat roundabout way of doing this. > > Thanks. > Sure we can merge them. That's fine. I can send a V4 or if you want to just squash them together that's fine. Jeff

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff Hostetler <[email protected]> writes: >>> Reviewed-by: Josh Steadmon <[email protected]> >> Let's see what JeffH says about this. I agree with you that making >> some stuff redundant in [Patch 3/4] and fixing the redundancy in >> this step does feel somewhat roundabout way of doing this. >> Thanks. >> > > Sure we can merge them. That's fine. I can send a V4 or if you want > to just squash them together that's fine. Let's have a v4 describing the change for combined 3&4 in your words, with Josh's Reviewed-by: already added to the trailers. Thanks, both of you.

gitgitgadget · 2024-03-06T21:49:14Z

User Josh Steadmon <[email protected]> has been added to the cc: list.

gitgitgadget · 2024-03-06T22:07:18Z

This patch series was integrated into seen via git@4082b9e.

gitgitgadget · 2024-03-06T22:58:32Z

User Jeff Hostetler <[email protected]> has been added to the cc: list.

gitgitgadget · 2024-03-07T00:22:02Z

This patch series was integrated into seen via git@6b2e4ea.

gitgitgadget · 2024-03-07T01:29:30Z

This patch series was integrated into seen via git@b110890.

Some commands do not cause a set of 'def_param' events to be emitted. This includes "git-remote-https", "git-http-fetch", and various "query" commands, like "git --man-path". Since all of these commands do emit a 'cmd_name' event, add code to the "trace2_cmd_name()" function to generate the set of 'def_param' events. Remove explicit calls to "trace2_cmd_list_config()" and "trace2_cmd_list_env_vars()" in git.c since they are no longer needed. Reviewed-by: Josh Steadmon <[email protected]> Signed-off-by: Jeff Hostetler <[email protected]>

jeffhostetler · 2024-03-07T15:21:10Z

/submit

gitgitgadget · 2024-03-07T15:22:40Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1679/jeffhostetler/always-emit-def-param-v2

To fetch this version to local tag pr-1679/jeffhostetler/always-emit-def-param-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1679/jeffhostetler/always-emit-def-param-v2

gitgitgadget · 2024-03-08T01:17:13Z

This patch series was integrated into seen via git@dde6457.

gitgitgadget · 2024-03-08T02:33:58Z

There was a status update in the "Cooking" section about the branch jh/trace2-missing-def-param-fix on the Git mailing list:

Some trace2 events that lacked def_param have learned to show it,
enriching the output.

Reviewed-by: Josh Steadmon <[email protected]>
cf. <[email protected]>

Will merge to 'next'.
source: <[email protected]>

gitgitgadget · 2024-03-09T02:18:24Z

This patch series was integrated into seen via git@a70c6fa.

gitgitgadget · 2024-03-09T02:18:24Z

This patch series was integrated into next via git@a797cfe.

gitgitgadget · 2024-03-10T03:54:07Z

This patch series was integrated into seen via git@88a6252.

gitgitgadget · 2024-03-12T00:46:18Z

This patch series was integrated into seen via git@75b82b4.

gitgitgadget · 2024-03-12T01:00:14Z

There was a status update in the "Cooking" section about the branch jh/trace2-missing-def-param-fix on the Git mailing list:

Some trace2 events that lacked def_param have learned to show it,
enriching the output.

Reviewed-by: Josh Steadmon <[email protected]>
cf. <[email protected]>

Will merge to 'master'.
source: <[email protected]>

gitgitgadget · 2024-03-14T00:10:29Z

This patch series was integrated into seen via git@9084feb.

gitgitgadget · 2024-03-14T21:46:00Z

This patch series was integrated into seen via git@5803d1d.

gitgitgadget · 2024-03-15T23:52:56Z

This patch series was integrated into seen via git@819e86c.

gitgitgadget · 2024-03-16T00:31:09Z

There was a status update in the "Cooking" section about the branch jh/trace2-missing-def-param-fix on the Git mailing list:

Some trace2 events that lacked def_param have learned to show it,
enriching the output.

Reviewed-by: Josh Steadmon <[email protected]>
cf. <[email protected]>

Will merge to 'master'.
source: <[email protected]>

gitgitgadget · 2024-03-16T19:09:17Z

This patch series was integrated into seen via git@cc31cc5.

gitgitgadget · 2024-03-18T20:59:11Z

This patch series was integrated into seen via git@7f1e926.

gitgitgadget · 2024-03-18T20:59:12Z

This patch series was integrated into master via git@7f1e926.

gitgitgadget · 2024-03-18T20:59:12Z

This patch series was integrated into next via git@7f1e926.

gitgitgadget · 2024-03-18T20:59:14Z

Closed via 7f1e926.

jeffhostetler self-assigned this Feb 29, 2024

jeffhostetler mentioned this pull request Feb 29, 2024

Clarity about detail levels git-ecosystem/trace2receiver#19

Closed

jeffhostetler force-pushed the always-emit-def-param branch from 8967b38 to 7612b3e Compare March 1, 2024 19:30

jeffhostetler force-pushed the always-emit-def-param branch from 7612b3e to aae7007 Compare March 1, 2024 19:33

dscho mentioned this pull request Mar 3, 2024

Fix make -C t test-lint-shell-syntax jeffhostetler/git#13

Merged

jeffhostetler added 2 commits March 4, 2024 09:58

jeffhostetler force-pushed the always-emit-def-param branch from 70cb3a7 to e852871 Compare March 4, 2024 15:01

gitgitgadget bot added the seen label Mar 4, 2024

gitgitgadget bot reviewed Mar 6, 2024

View reviewed changes

jeffhostetler force-pushed the always-emit-def-param branch from e852871 to 178721c Compare March 7, 2024 15:12

jeffhostetler changed the title ~~trace2: move 'def_param' events into 'cmd_name' and 'cmd_alias'~~ trace2: move generation of 'def_param' events into code for 'cmd_name' Mar 7, 2024

gitgitgadget bot added the next label Mar 9, 2024

gitgitgadget bot added the master label Mar 18, 2024

gitgitgadget bot closed this Mar 18, 2024

		@@ -373,8 +373,6 @@ static int handle_alias(int argcp, const char **argv)
		strvec_pushv(&child.args, (*argv) + 1);

trace2: move generation of 'def_param' events into code for 'cmd_name' #1679

trace2: move generation of 'def_param' events into code for 'cmd_name' #1679

Uh oh!

Conversation

jeffhostetler commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitgitgadget bot commented Mar 1, 2024

Uh oh!

dscho commented Mar 3, 2024

Uh oh!

gitgitgadget bot commented Mar 4, 2024

Uh oh!

jeffhostetler commented Mar 4, 2024

Uh oh!

gitgitgadget bot commented Mar 4, 2024

Uh oh!

gitgitgadget bot commented Mar 4, 2024

Uh oh!

gitgitgadget bot commented Mar 4, 2024

Uh oh!

gitgitgadget bot commented Mar 4, 2024

Uh oh!

gitgitgadget bot commented Mar 5, 2024

Uh oh!

gitgitgadget bot Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot commented Mar 6, 2024

Uh oh!

gitgitgadget bot commented Mar 6, 2024

Uh oh!

gitgitgadget bot commented Mar 6, 2024

Uh oh!

gitgitgadget bot commented Mar 7, 2024

Uh oh!

gitgitgadget bot commented Mar 7, 2024

Uh oh!

jeffhostetler commented Mar 7, 2024

Uh oh!

gitgitgadget bot commented Mar 7, 2024

Uh oh!

gitgitgadget bot commented Mar 8, 2024

Uh oh!

gitgitgadget bot commented Mar 8, 2024

Uh oh!

gitgitgadget bot commented Mar 9, 2024

Uh oh!

gitgitgadget bot commented Mar 9, 2024

Uh oh!

gitgitgadget bot commented Mar 10, 2024

Uh oh!

gitgitgadget bot commented Mar 12, 2024

Uh oh!

gitgitgadget bot commented Mar 12, 2024

Uh oh!

gitgitgadget bot commented Mar 14, 2024

Uh oh!

gitgitgadget bot commented Mar 14, 2024

Uh oh!

gitgitgadget bot commented Mar 15, 2024

Uh oh!

gitgitgadget bot commented Mar 16, 2024

Uh oh!

gitgitgadget bot commented Mar 16, 2024

Uh oh!

gitgitgadget bot commented Mar 18, 2024

Uh oh!

gitgitgadget bot commented Mar 18, 2024

Uh oh!

gitgitgadget bot commented Mar 18, 2024

jeffhostetler commented Feb 29, 2024 •

edited

Loading