Re: [PATCH v3 1/1] fetch: add trace2 instrumentation

From: Erik Chen <erikchen@chromium.org>
Date: Nov 19, 2019, 21:51 UTC
Message-ID: <c2cd57ed-8075-bdd3-67f0-21187580270a@chromium.org>
In-Reply-To: <944c956e-dd9a-1a12-5cb1-0c263ee7d5bd@gmail.com>

Importing response from https://github.com/gitgitgadget/git/pull/451#issuecomment-555077933

> I notice that a v4 was sent that adds more sub-regions without actually
> responding to this request.

Sorry, did I misunderstand the request? I was trying to implement the suggestion: <Junio's first suggestion>

 > (It is worth also pointing out that you
 > ignored Junio's request you use the cover letter to explain your 
reasoning
 > for changes between versions.)
Sorry, this was not intentional. I'm using GitGitGadget, and it wasn't 
clear to me how to change this. See my comment here:
<https://github.com/gitgitgadget/git/pull/451#issuecomment-555044068>

Show 8 quoted lines

> 
> There is a real downside to nesting regions like this. Specifically, we
> frequently limit the depth that we report nested regions to avoid
> overwhelming the logs.
> 
> In general, these sub-regions should be avoided when possible and instead
> create regions around important sections, such as the second option Junio
> lists above.

I thought it would be nice to have an encapsulating region for the full function, but I can switch to Junio's second option if that's preferable. It wasn't clear to me that that was preferred over the first option?