Re: POC: Extension for adding distributed tracing - pg_tracing

Hi!
Great you continue to work on this patch!
I'm checking out the newest changes.

On Fri, Sep 15, 2023 at 2:32 PM Aleksander Alekseev <
aleksander(at)timescale(dot)com> wrote:

> Hi,
>
> > Renaming/Refactoring:
> > - All spans are now tracked in the palloced current_trace_spans buffer
> > compared to top_span and parse_span being kept in a static variable
> > before.
> > - I've renamed query_spans to top_span. A top_span serves as the
> > parent for all spans in a specific nested level.
> > - More debugging information and assertions. Spans now track their
> > nested level, if they've been ended and if they are a top_span.
> >
> > Changes:
> > - I've added the subxact_count to the span's metadata. This can help
> > identify the moment a subtransaction was started.
> > - I've reworked nested queries and utility statements. Previously, I
> > made the assumptions that we could only have one top_span per nested
> > level which is not the case. Some utility statements can execute
> > multiple queries in the same nested level. Tracing utility statement
> > now works better (see image of tracing a create extension).
>
> Many thanks for the updated patch.
>
> If it's not too much trouble, please use `git format-patch`. This
> makes applying and testing the patch much easier. Also you can provide
> a commit message which 1. will simplify the work for the committer and
> 2. can be reviewed as well.
>
> The tests fail on CI [1]. I tried to run them locally and got the same
> results. I'm using full-build.sh from this repository [2] for
> Autotools and the following one-liner for Meson:
>
> ```
> time CPPFLAGS="-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS" sh -c 'git
> clean -dfx && meson setup --buildtype debug -Dcassert=true
> -DPG_TEST_EXTRA="kerberos ldap ssl load_balance" -Dldap=disabled
> -Dssl=openssl -Dtap_tests=enabled
> -Dprefix=/home/eax/projects/pginstall build && ninja -C build && ninja
> -C build docs && PG_TEST_EXTRA=1 meson test -C build'
> ```
>
> The comments for pg_tracing.c don't seem to be formatted properly.
> Please make sure to run pg_indent. See src/tools/pgindent/README
>
> The interface pg_tracing_spans(true | false) doesn't strike me as
> particularly readable. Maybe this function should be private and the
> interface be like pg_tracing_spans() and pg_trancing_consume_spans().
> Alternatively you could use named arguments in the tests, but I don't
> think this is a preferred solution.
>
> Speaking of the tests I suggest adding a bit more comments before
> every (or most) of the queries. Figuring out what they test could be
> not particularly straightforward for somebody who will make changes
> after the patch will be accepted.
>
> [1]: http://cfbot.cputube.org/
> [2]: https://github.com/afiskon/pgscripts/
>
> --
> Best regards,
> Aleksander Alekseev
>

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/