Add ability to set arbitrary external acquire duration observer

[sashayakovtseva]

This PR is a rework of #29.

No extra dependencies are added, no dependency updates are performed.
These changes do not break backward compatibility, external observer is optional and must be explicitly set by users.

I see no other way to measure acquire duration correctly.
I understand the current status of this project and won't bother you with any other PRs if this one is not considered.

[jackc]

This is definitely an improvement and I think it would be reasonable to include something like this in puddle. But it occurred to me that with a few changes it could be even more flexible and useful.

Instead of "metrics" callback(s), what if it was a tracing system like pgx has? Then it could integrate with things like opentelemetry as well.

The same tracing object used in pgx could implement additional interface(s) and record additional information. It would need a little integration work with pgxpool as well, but it shouldn't be onerous.

---

The below might not be applicable if we switch to a tracing interface, but I'll mention them anyway.

It appears that the builtin stats are implemented with the metrics system now. AFAICT, this means that if custom metrics are assigned that the standard builtin stats are disabled. I'm not sure how I feel about that. It's not exactly a backwards incompatible change, but it could be surprising.

I'm not a big fan of the options pattern used by NewMetrics (even though it is used a few places in pgx). Unless there is a special reason I lean toward simple struct fields.

[sashayakovtseva]

Hi @jackc, thanks for your reply

It appears that the builtin stats are implemented with the metrics system now. AFAICT, this means that if custom metrics are assigned that the standard builtin stats are disabled. I'm not sure how I feel about that. It's not exactly a backwards incompatible change, but it could be surprising.

Nope, standard builtins are not disabled. Standard counters are types that are valid to use when initialized with zero value (int64, time.Duration which is int64 as well, atomic.Int64).
I've already tested custom metrics assignment here. Both old metrics, observed by https://github.com/IBM/pgxpoolprometheus, and my new metric are collected into Prom correctly.

Instead of "metrics" callback(s), what if it was a tracing system like pgx has? Then it could integrate with things like opentelemetry as well.

That looks like a more generic and flexible solution. I think I can draft another PR rework soon, need to investigate that approach first.

[sashayakovtseva]

Do I understand your general idea correctly?

1. introduce new interface to puddle package to support opentelemetry metrics. It might look something like this:

type AcquireDurationData struct {
    D time.Duration
    IsEmptyAcquire bool
}

type CanceledAquireData struct {} // I'm not sure we need anything here b/c this counter goes up by 1 only

type MetricsRecorder interface {
    RecordAcquireDuration(context.Context, AcquireDurationData)
    AddCaceledAcquire(context.Context, CanceledAquireData)
}

2. get rid of Metrics struct (or make it unexported? this way we can change built-in counters and invoke recorder in a single place and do not overwhelm acquire function) and instead add MetricsRecorder field in Config
3. invoke MetricsRecorder where necessary

Regarding tracing support, do we need it in puddle? I feel like metrics cover all the requested observability and there's not much to trace, actually.

[jackc]

What I was picturing was more like:

type AcquireTracer interface {
	AcquireStart(ctx context.Context, data AcquireStartData) context.Context
	AcquireEnd(ctx context.Context, data AcquireEndData)
}

// Maybe also have a ReleaseTracer or maybe there should be a single Checkout or Pool tracer that has more trace points.

Tracing provides a superset of metrics functionality. It can be used for things like the original histogram feature, but it also would allow much more. For example, it would be possible to see exactly how long a query request was waiting for an available query and how long was spent actually performing the query. It could also be used to distinguish between queries canceled while in progress and queries canceled before a connection was available.

I haven't thought through exactly what are the proper trace points, but I am fairly sure that separate "begin" and "end" events are more flexible than only making the results of the acquire attempt available.

[sashayakovtseva]

Got it, makes sense.
I think maybe we can start with PoolTracer which has acquire end/start and then extend it with other trace points when necessary?

[jackc]

I think we would also want something to trace Release.