client_golang icon indicating copy to clipboard operation
client_golang copied to clipboard

Improve Exemplar Interface

Open bwplotka opened this issue 2 years ago • 7 comments

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:

  • We want to observe with a trace ID.
  • We want a consistent trace ID label.
  • We want trace ID which is sampled, no point in any other (ofc won't work with tail sampling)
  • Assumption: Empty Trace ID means it's not sampled.

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

bwplotka avatar May 03 '22 10:05 bwplotka

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 avatar May 03 '22 10:05 beorn7

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

beorn7 avatar May 03 '22 10:05 beorn7

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?

kakkoyun avatar May 03 '22 13:05 kakkoyun

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

bboreham avatar May 23 '22 14:05 bboreham

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] avatar Nov 23 '22 03:11 stale[bot]

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

stale[bot] avatar Apr 02 '23 13:04 stale[bot]

Still relevant (:

bwplotka avatar Apr 03 '23 09:04 bwplotka