git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Stefan Beller <sbeller@google.com>
To: gitgitgadget@gmail.com
Cc: git <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>,
	Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH 1/8] trace2: create new combined trace facility
Date: Tue, 4 Sep 2018 15:12:53 -0700	[thread overview]
Message-ID: <CAGZ79kbUYDAKi-K2uHpkffPjMxGYtH=QUMhvfq4HTc5+a7-eBA@mail.gmail.com> (raw)
In-Reply-To: <82885700379efe6d6a83629cac4d943b99b393bf.1535734192.git.gitgitgadget@gmail.com>

> Create GIT_TR2 trace-key to replace GIT_TRACE, GIT_TR2_PERFORMANCE to
> replace GIT_TRACE_PERFORMANCE, and a new trace-key GIT_TR2_EVENT to
> generate JSON data for telemetry purposes.  Other structured formats
> can easily be added later using this new existing model.

So the idea is to use the GIT_TR2 instead of GIT_TRACE and we
get the same output as well as a new form of structured logging here?
(Then GIT_TRACE could be retired, and we'd use the new API to add
more events, which are also more structured as the API allows for more
than just a string printed?)

> Define a higher-level event API that selectively writes to all of the
> new GIT_TR2_* targets (depending on event type) without needing to call
> different trace_printf*() or trace_performance_*() routines.
>
> The API defines both fixed-field and printf-style functions.
>
> The trace2 performance tracing includes thread-specific function
> nesting and timings.

So this only adds the new API, and we need to merge the TRACE
into the TRACE2 later?

> +++ b/trace2.c
> @@ -0,0 +1,1592 @@
[...]
> +
> +/*****************************************************************
> + * TODO remove this section header
> + *****************************************************************/

Yes, please.

> +/*
> + * Compute a "unique" session id (SID) for the current process.  All events
> + * from this process will have this label.  If we were started by another
> + * git instance, use our parent's SID as a prefix and count the number of
> + * nested git processes.  (This lets us track parent/child relationships
> + * even if there is an intermediate shell process.)

How does this work with threading. From this description we can have
two threads starting new child processes and they have the same ID
(<our id>-2)

> +
> +/*****************************************************************
> + * TODO remove this section header
> + *****************************************************************/

This looks like a reoccurring pattern. Did you have a tool
that needs these? Does the tool want to enforce some level of
documentation on each function?

> +
> +/*
> + * Each thread (including the main thread) has a stack of nested regions.
> + * This is used to indent messages and provide nested perf times.  The
> + * limit here is for simplicity.  Note that the region stack is per-thread
> + * and not per-trace_key.

What are the (nested) regions used for? I would imagine recursive
operations, e.g. unpacking trees? Where am I supposed to read
up on those?

> + */
> +#define TR2_MAX_REGION_NESTING (100)
> +#define TR2_INDENT (2)
> +#define TR2_INDENT_LENGTH(ctx) (((ctx)->nr_open_regions - 1) * TR2_INDENT)

In the structured part of the logging the indentation would not be
needed and we'd rather want to store the nesting level as an int,
this is solely for printing locally I'd assume.

> +#define TR2_MAX_THREAD_NAME (24)

This is the max length for a thread name including all the prefixes?


> +static void tr2tls_unset_self(void)
> +{
> +       struct tr2tls_thread_ctx * ctx;
> +
> +       ctx = tr2tls_get_self();
> +
> +       pthread_setspecific(tr2tls_key, NULL);

Would we also need to free tr2tls_key ?


> +/*****************************************************************
> + * TODO remove this section header
> + *****************************************************************/
> +
> +static void tr2io_write_line(struct trace_key *key, struct strbuf *buf_line)
> +{
> +       strbuf_complete_line(buf_line); /* ensure final NL on buffer */
> +
> +       // TODO we don't really want short-writes to try again when
> +       // TODO we are in append mode...

A different kind of TODO in a // comment?

  parent reply	other threads:[~2018-09-04 22:13 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-31 16:49 [PATCH 0/8] WIP: trace2: a new " Jeff Hostetler via GitGitGadget
2018-08-31 16:49 ` [PATCH 1/8] trace2: create new combined " Jeff Hostetler via GitGitGadget
2018-08-31 17:19   ` Derrick Stolee
2018-09-04 22:12   ` Stefan Beller [this message]
2018-09-04 22:30     ` Junio C Hamano
2018-09-05 15:51       ` Jeff Hostetler
2018-09-05 15:01     ` Jeff Hostetler
2018-08-31 16:49 ` [PATCH 2/8] trace2: add trace2 to main Jeff Hostetler via GitGitGadget
2018-08-31 16:49 ` [PATCH 3/8] trace2: demonstrate trace2 regions in wt-status Jeff Hostetler via GitGitGadget
2018-08-31 16:49 ` [PATCH 4/8] trace2: demonstrate trace2 child process classification Jeff Hostetler via GitGitGadget
2018-08-31 16:50 ` [PATCH 5/8] trace2: demonstrate instrumenting do_read_index Jeff Hostetler via GitGitGadget
2018-08-31 16:50 ` [PATCH 6/8] trace2: demonstrate instrumenting threaded preload_index Jeff Hostetler via GitGitGadget
2018-08-31 16:50 ` [PATCH 7/8] trace2: demonstrate setting sub-command parameter in checkout Jeff Hostetler via GitGitGadget
2018-08-31 16:50 ` [PATCH 8/8] trace2: demonstrate use of regions in read_directory_recursive Jeff Hostetler via GitGitGadget
2018-08-31 17:19 ` [PATCH 0/8] WIP: trace2: a new trace facility Derrick Stolee
2018-09-06 15:13   ` [RFC PATCH 0/6] Use trace2 in commit-reach Derrick Stolee
2018-09-06 15:13     ` [RFC PATCH 1/6] commit-reach: add trace2 telemetry and walk count Derrick Stolee
2018-09-06 15:13     ` [RFC PATCH 2/6] comit-reach: use trace2 for commit_contains_tag_algo Derrick Stolee
2018-09-06 15:13     ` [RFC PATCH 3/6] commit-reach: use trace2 in can_all_from_reach Derrick Stolee
2018-09-06 15:13     ` [RFC PATCH 4/6] test-tool: start trace2 environment Derrick Stolee
2018-09-06 15:13     ` [RFC PATCH 5/6] test-lib: add run_and_check_trace2 Derrick Stolee
2018-09-06 15:13     ` [RFC PATCH 6/6] commit-reach: fix first-parent heuristic Derrick Stolee
2018-10-11  1:50       ` Jonathan Nieder
2018-10-11 11:00         ` Derrick Stolee
2019-01-15  1:05 ` [PATCH 0/8] WIP: trace2: a new trace facility Jonathan Nieder
2019-01-15 17:03   ` Jeff Hostetler

Reply instructions:

You may reply publicly 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='CAGZ79kbUYDAKi-K2uHpkffPjMxGYtH=QUMhvfq4HTc5+a7-eBA@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --subject='Re: [PATCH 1/8] trace2: create new combined trace facility' \
    /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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).