Re: POC: Extension for adding distributed tracing - pg_tracing

From: Anthonin Bonnefoy <anthonin(dot)bonnefoy(at)datadoghq(dot)com>
To: Nikita Malakhov <hukutoc(at)gmail(dot)com>
Cc: Aleksander Alekseev <aleksander(at)timescale(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: POC: Extension for adding distributed tracing - pg_tracing
Date: 2023-09-22 15:45:27
Message-ID: CAO6_Xqoz8htXYGd9q7_G1c=Y2pWWAJhcY0EaXLNFz+f-bbdn4Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi!

> 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.
No problem, I will do that from now on.

> The tests fail on CI [1]. I tried to run them locally and got the same
> results.
I've fixed the compilation issue, my working laptop is a mac so I've missed the
linux errors. I've set up the CI on my side to avoid those issues in the future.
I've also fixed most of the test issues.
One test is still flaky: the parallel worker test can have one less
worker reporting
from time to time, I will try to find a way to make it more stable.

> 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
Done.

> 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().
Agree. I wasn't super happy about the format. I've created
pg_tracing_peek_spans() and pg_tracing_consume_spans() to make it
more explicit.

> 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.
I've added more comments and a representation of the expected span
hierarchy for the tests involving nested queries. It helps me a lot to keep
track of the spans to check.

I've done some additional changes:
- I've added a start_ns to spans which is the nanosecond remainder of the
time since the start of the trace. Since some spans can have a very short
duration, some spans would have the same start if we use a microsecond
precision. Having a start with a nanosecond precision allowed me to remove
some hacks I was using and makes tests more stable.
- I've also added a start_ns_time to span which is the value of the monotonic
clock at the start of the span. I used to use duration_ns as a
temporary storage
for this value but it made things more confusing and hard to follow.
- Fix error handling with nested queries created with ProcessUtility (like
calling "create extension pg_tracing" twice).

Regards,
Anthonin

Attachment Content-Type Size
pg-tracing-v7.patch application/octet-stream 199.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Matthias van de Meent 2023-09-22 15:50:08 Re: GenBKI emits useless open;close for catalogs without rows
Previous Message Daniel Gustafsson 2023-09-22 15:29:01 Re: GUC for temporarily disabling event triggers