Add Prometheus integration

[cyriltovena]

What do we think about adding Prometheus integration to this library.

It seems hard to do outside of the library. Something I could contribute if needed.

[cristaloleg]

Hi @cyriltovena Agree that observability is a good thing and that's why Stats where added (https://github.com/cristalhq/hedgedhttp/blob/main/hedged.go#L27) but what is the current problem with the metrics?

I'm kinda happy to keep this library without dependencies (also Prometheus client isn't a light one 😢 ) but not going to say "NO, NO PROM PLEASE" without full picture :D

Also check this one, I thought same pattern can be used with Stats mentioned above (just fetch stats in a loop with sleep and push to Prom or other metrics client), example: https://github.com/cristalhq/sqlmetrics

[cyriltovena]

The collector pattern could work, that was my first lead, but I figured it's hard to know when a transport is not used anymore and so when to stop polling it.

Not going to say it's impossible, but a global stats would have been easier IMO.

[cristaloleg]

Hm, that's interesting. How the global metrics will solve this problem? I understand that for your case global is okay ('cause it's used in only 1 app) but what will happen when this lib is used by 2 other libraries by example. Am I missing something?

[cyriltovena]

I don't disagree but our usage makes it hard for sure.

[cyriltovena]

For example in tempo this is how the stats is used: https://github.com/grafana/tempo/pull/790/files#diff-b1699c73b6dad09e708e097215ab78277d0d5c75f816463aa94de2842c09ab97R26

I'm concerned about leaking the goroutine and the ticker.

[cyriltovena]

I might be able to use finalizer for my use case.

[cristaloleg]

Oh, you've closed the issue before I asked 1 more question :( Anyway :)

I figured it's hard to know when a transport is not used anymore and so when to stop polling it.

Aren't you controlling where you're using hedged client? So basically you've:

package tempo_hedged_client

func NewClient(ctx context.Context) (*HedgedClient, error) {
    client, stats := hedhedhttp. NewClientAndStats(..)
	go func() {
		for {
			select {
				case <-ctx.Done(): return
				case <-pollInterval: // submit stats to Prom-like
		}
	}()
    return client, ...
}

package storage

func ...() {
	// some ctx that you can control
    client, err := NewClient(ctx)
}

And just with the slightly propagated ctx you're in control of polling goroutine. Am I missing something?

As in your example (https://github.com/grafana/tempo/blob/main/tempodb/backend/instrumentation/hedged_requests.go#L26) you can just add context param to PublishHedgedMetrics and control this polling goroutine. Jumping through the code a bit have found https://github.com/grafana/tempo/blob/ba93a400a4968c303cdbff9f5ccf88f6194489ca/tempodb/backend/s3/s3.go#L255 where you can cancel such polling context (AFAIR this method belongs to a core interface in Grafana project(s)).