Re: POC: Extension for adding distributed tracing - pg_tracing

Hi,

> I think one hugely important benefit of using GUCs over sql comments
> is, that comments and named protocol-level prepared statements cannot
> be combined afaict. Since with named protocol level prepared
> statements no query is sent, only arguments. So there's no place to
> attach a comment too.

Handling named prepared statements has definitely been a pain point.
I've been experimenting and it is possible to combine comments with
prepared statements by passing the comment's content as a parameter.

PREPARE test_prepared (text, integer) AS /*$1*/ SELECT $2;
EXECUTE test_prepared('dddbs=''postgres.db'',traceparent=''00-00000000000000000000000000000009-0000000000000009-01''',
1);

Though adding a parameter is definitely more intrusive than just
adding a comment and this is not defined and supported anywhere. I
will try to see how much work and complexity it would involve to add
the trace context propagation through GUC variables.
On one hand, having more options would allow more flexibility and ease
to implement the trace propagation. On the other hand, I would like to
keep things simple for a first iteration.

> One of the test has failed in CFBOT [1] with

I've identified and fixed the issue. An assertion checking that spans
generated from planstate didn't end before the parent's end could be
triggered.

The new patch also contains the following changes:
- Support for 128 bits trace ids. While w3c defines exceptions to
allow compatibility with systems that only support 64 bits trace ids,
supporting 128 bits trace ids early should remove the possible pain of
migrating or interoperating issues.
- Trace ids, span ids and parent ids outputs are now hexadecimal
characters instead of bigints. This removes the negative ids issue due
to not having unsigned bigint and was necessary for the 128 bits trace
id support. It also matches the format that's propagated through the
traceparent value. They are still stored as uint64 to keep memory
footprint low.
- Added support for before trigger.
- Refactored all headers to a single header file.
- Added Query id filtering. Adding this turned out to be rather straightforward.
- Additional edge cases found and fixed (planner error could trigger
an assertion failure).

Regards,
Anthonin

On Sun, Jan 7, 2024 at 12:48 AM Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:
>
> On Thu, Jan 4, 2024, 16:36 Anthonin Bonnefoy
> <anthonin(dot)bonnefoy(at)datadoghq(dot)com> wrote:
> > > I used GUCs to set the `opentelemtery_trace_id` and `opentelemetry_span_id`.
> > > These can be sent as protocol-level messages by the client driver if the
> > > client driver has native trace integration enabled, in which case the user
> > > doesn't need to see or care. And for other drivers, the application can use
> > > `SET` or `SET LOCAL` to assign them.
> >
> > Emitting `SET` and `SET LOCAL` for every traced statement sounds more
> > disruptive and expensive than relying on SQLCommenter. When not using
> > `SET LOCAL`, the variables also need to be cleared.
> > This will also introduce a lot of headaches as the `SET` themselves
> > could be traced and generate spans when tracing utility statements is
> > already tricky enough.
>
> I think one hugely important benefit of using GUCs over sql comments
> is, that comments and named protocol-level prepared statements cannot
> be combined afaict. Since with named protocol level prepared
> statements no query is sent, only arguments. So there's no place to
> attach a comment too.