Re: [PATCH v2 6/8] serve.c: add trace2 regions for advertise & command

From: Jeff King <peff@peff.net>
Date: Jul 1, 2021, 16:30 UTC
Message-ID: <YN3tjEE8Ir7Zuvkq@coredump.intra.peff.net>
In-Reply-To: <patch-6.8-baeee6539ad-20210628T191634Z-avarab@gmail.com>

On Mon, Jun 28, 2021 at 09:19:23PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Now that we've factored out "call_{advertise,command}()" in a
> preceding commit it becomes easy to trace all these callbacks with
> trace2. Let's do that. As the tests demonstrate there's no v2 push
> protocol, which the tests assert.

Seems reasonable. I haven't ever wanted these myself, but it seems like a natural spot to mention when debugging server-side actions (especially because we may get multiple rounds of "fetch" for a single upload-pack invocation).

Show 19 quoted lines

> diff --git a/serve.c b/serve.c
> index 85cd3eab26e..6dbd05248b9 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -111,14 +111,34 @@ static struct protocol_capability capabilities[] = {
>  static int call_advertise(struct protocol_capability *command,
>  			  struct repository *r, struct strbuf *value)
>  {
> -	return command->advertise(r, value);
> +	int ret;
> +	struct strbuf sb = STRBUF_INIT;
> +	const char *msg;
> +
> +	strbuf_addf(&sb, "advertise/%s", command->name);
> +	trace2_region_enter("serve", sb.buf, r);
> +	ret = command->advertise(r, value);
> +	msg = ret ? "advertised" : "hidden";
> +	trace2_region_leave_printf("serve", sb.buf, r, "%s", msg);
> +	strbuf_release(&sb);

We'll do these allocations even if trace2 isn't enabled. I guess that's probably not that big a deal in practice. I think:

  if (trace2_is_enabled())
	strbuf_addf(&sb, "advertise/%s", command->name);

would work (everything else is cheap and handles the unallocated state fine), but it might not be worth the readability hit (and it's probably premature optimization anyway).

-Peff