prometheus / client_golang

Prometheus instrumentation library for Go applications
https://pkg.go.dev/github.com/prometheus/client_golang
Apache License 2.0
5.34k stars 1.17k forks source link

Improve Exemplar Interface #1042

Open bwplotka opened 2 years ago

bwplotka commented 2 years ago

Using exemplars is quite painful currently. There is a common pattern you want to do such as:

image

I would propose something more natively supported. Some options:

A) Bring ObserveWithExemplar to Observer, why not? B) Create a separate package that creates natively observers with exemplars (sounds brittle and more to maintain for not a good reason) C) Create a helper that does something like:

func ExemplarObserve(obs prometheus.Observer, val float64, traceID string) {
    if traceID != "" {
        obs.(prometheus.ExemplarObserver).ObserveWithExemplar(
            val, map[string]string{"trace-id": traceID})
    } else {
        obs.Observe(val)
    }
}

The opinionated state is simple:

Ideally we have opinionated helper as part of method to Observe itself with configurable trace ID label... 🙈

Having if else when histogram is used is making client instrumentation too verbose IMO. Let's brainstorm this (:

Thoughts? @kakkoyun @juliusv @beorn7

beorn7 commented 2 years ago

A) Bring ObserveWithExemplar to Observer, why not?

Because it changes the Observer interface. It breaks everyone who has implemented the Observer interface outside of our control (most commonly, I guess, when creating mocks for testing, but there might be other use cases). And yes, this might be a fairly niche part of the user community, but since this package is used so widely, it will still create a fair amount of breakage and confusion and destroy the trust people have developed into this package.

This reason, BTW, was the most important one for picking the interface upgrade path when implementing exemplar support.

beorn7 commented 2 years ago

The other approaches are all valid options, I think. I would just generally suggest to put helpers into a separate package. My experience from the past is that users get really confused and frustrated if a package has too many top-level types. It's a lot of cognitive load when learning how to use the package. Therefore, I had spent a lot of effort in the past to reduce the number of top-level types in the prometheus package. Note that the "group helpers for specific use cases in appropriate packages" pattern has been followed before (testutil, collectors, promhttp, promauto).

kakkoyun commented 2 years ago

I would also vote for a dedicated package to contain new helper functions around exemplars. We can even have high-level interface that would conform span/context from popular tracing packages or OTel. (I haven't checked their types if it's even possible IsSampled and TraceID seems like good candidates for that imagined interface). What do you think?

bboreham commented 2 years ago

MBOI we have been using this helper "CollectedRequest" for a few years: it times the request, collects the histogram observation, and now adds exemplars: https://github.com/weaveworks/common/blob/bd288de53d57de300fa286688ce2fc935687213f/http/client/client.go#L66-L67

stale[bot] commented 1 year ago

Hello 👋 Looks like there was no activity on this issue for the last 3 months. Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗 If there will be no activity in the next 4 weeks, this issue will be closed (we can always reopen an issue if we need!).

stale[bot] commented 1 year ago

Closing for now as promised, let us know if you need this to be reopened! 🤗

bwplotka commented 1 year ago

Still relevant (: