Re: [PATCH] upload-pack: add tracing for fetches

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 11, 2023, 21:32 UTC
Message-ID: <ZScUWrj6CQTg05HL@nand.local>
In-Reply-To: <pull.1598.git.1697040242703.gitgitgadget@gmail.com>

On Wed, Oct 11, 2023 at 04:04:02PM +0000, Robert Coup via GitGitGadget wrote:

Show 6 quoted lines

> From: Robert Coup <robert@coup.net.nz>
>
> Information on how users are accessing hosted repositories can be
> helpful to server operators. For example, being able to broadly
> differentiate between fetches and initial clones; the use of shallow
> repository features; or partial clone filters.

Indeed. One of the custom patches that GitHub is carrying in its private fork is something similar to this that dumps information from upload-pack into a custom logging system specific to GitHub (called "sockstat", in case anybody was curious).

I suspect that we would still live with that patch because we depend on some of the custom logging infrastructure provided by sockstat, but this is definitely a good direction to be pursuing for git.git nonetheless.

Show 10 quoted lines

> @@ -1552,6 +1553,32 @@ static int parse_have(const char *line, struct oid_array *haves)
>  	return 0;
>  }
>
> +static void trace2_fetch_info(struct upload_pack_data *data)
> +{
> +	struct json_writer jw = JSON_WRITER_INIT;
> +
> +	jw_object_begin(&jw, 0);
> +	{

Is there a reason that we have a separate scope here? I think we may want to drop this as unnecessary, but it's entirely possible that I'm missing something here...

Show 10 quoted lines

> +		jw_object_intmax(&jw, "haves", data->haves.nr);
> +		jw_object_intmax(&jw, "wants", data->want_obj.nr);
> +		jw_object_intmax(&jw, "want-refs", data->wanted_refs.nr);
> +		jw_object_intmax(&jw, "depth", data->depth);
> +		jw_object_intmax(&jw, "shallows", data->shallows.nr);
> +		jw_object_bool(&jw, "deepen-since", data->deepen_since);
> +		jw_object_intmax(&jw, "deepen-not", data->deepen_not.nr);
> +		jw_object_bool(&jw, "deepen-relative", data->deepen_relative);
> +		if (data->filter_options.choice)
> +			jw_object_string(&jw, "filter", list_object_filter_config_name(data->filter_options.choice));

I'm pretty sure that list_object_filter_config_name() returns characters that are safe for JSON-encoding, and/or that jw_object_string() does any quoting beforehand, but worth checking nonetheless.

> +		else
> +			jw_object_null(&jw, "filter");

These all seem like useful things to have.

Thanks, Taylor