Potentially deprecating

[ocharles]

After chatting with the folks over at #prometheus, they are very interested in there only being one client per language - and this makes sense. I re-evaluated prometheus-client, and I don't think there's enough in this library that warrants its continuing existence. I've rewritten some code that uses prometheus-effect to use prometheus-client, and for the most part - it's pretty trivial I would like to:

a. Improve the documentation of prometheus-client so it's at the same standard as prometheus-effect. b. Add the time, countExceptions and ioBuckets to prometheus-client. c. Add push support to prometheus-client, or perhaps a separate library.

@qrilka and @NicolasT - as contributors to this project, I'd love to hear your thoughts, and perhaps why you chose this library over prometheus-client.

@fimad and @jml, as contributors to prometheus-client, I'd also like to hear your thoughts.

[jml]

Thanks @ocharles. I really respect this decision.

• agree re one client for Haskell. there's also another prometheus haskell library floating about, but fimad's is probably the one to settle on
• I'm all in favour of improved docs
• I don't know what time, countExceptions, or ioBuckets do. Will take a look as soon as I'm able.
• No personal interest in push support (daemons or bust!), but knock yourself out.

I won't get started on my own wishlist for prometheus-client—will be here all night.

Any other things you want thoughts on?

[ocharles]

I don't know what time, countExceptions, or ioBuckets do. Will take a look as soon as I'm able.

• time times to an IO action to a histogram. There is something similar in prometheus-client but it times to a gauge.
• countExceptions runs an IO action, and bumps a counter if a synchronous exception is thrown (it rethrows the exception).
• ioBuckets is a predefined set of buckets for histograms, that works well for IO actions. There's a lot of granularity for < 1s, and then less up to 16s (exponential then linear, I think).

No personal interest in push support (daemons or bust!), but knock yourself out.

Need it when running on Heroku :(

I won't get started on my own wishlist for prometheus-client—will be here all night.

I would still be very interested in hearing about this though! If @fimad is up for it, I'd be willing to co-maintain prometheus-client.

[qrilka]

@ocharles I'm not against such a move but what about Prometheus.GHC?

[fimad]

@ocharles, sounds good to me. I'd be open to merging the missing functionality from prometheus-effect into prometheus-client and adding some other co-maintainers. Better documentation is always welcome :)

@qrilka, There is support for GHC metrics in prometheus-client available as a separate library, prometheus-metrics-ghc. You can see the currently exported metrics here.

[ocharles]

Great, I'll spend some time this weekend and see if I can put together a pull request for prometheus-client

[NicolasT]

I'll get back to this question later this weekend.

[NicolasT]

Why I chose this library: in some project I have some metrics collection, and various back-ends can be used to implement this. At first I only had an EKG backend, which I then propagated into Prometheus using ekg-prometheus-adapter, but it turns out EKG metrics and Prometheus metric types don't always nicely match, so I wanted a native Prometheus implementation.

So I went on Hackage, searched for Prometheus, looked at the 'last release' times, saw @ocharles as an author (which is a sign of trustworthiness to me), and that was mostly it. This package provides what I need (except for maybe #5 for which I now use a somewhat ugly work-around), and I didn't look any further. It also covers most if not all of what a Prometheus lib needs.

As for prometheus-client:

• There seems to be some kind of magic global registry, which is not very Haskell'y (and is likely the #1 issue I have with the API).
• I'm a bit puzzled by unsafeRegisterIO and unsafeRegister, not sure it's wise/useful to have that kind of thing.
• MonadMonitor is a bit odd, especially when e.g. addDurationToCounter and others returns IO ().
• Use of String instead of Text
• No built-in way to register the metrics in some Wai app, as far as I could see.

[ocharles]

Thanks for the high praise - I'm glad you find my work useful! I think you'll be roughly OK with swapping out my library for prometheus-client, there's nothing there that we can't have in the official client. I would hold off though for right now. I spent the weekend working on some changes to prometheus-client, and I want to see how the current maintainers feel, as there are some API breaking changes.

There seems to be some kind of magic global registry, which is not very Haskell'y (and is likely the #1 issue I have with the API).

Not very Haskelly, but from talking to the Prometheus developers themselves - that is how they want it to work. And really, it does make sense - the only thing that is more questionable is unsafe registration of metrics.

Use of String instead of Text

I'm working on switching this out.

No built-in way to register the metrics in some Wai app, as far as I could see.

http://hackage.haskell.org/package/wai-middleware-prometheus-0.2.0/docs/Network-Wai-Middleware-Prometheus.html

[jml]

Not very Haskelly, but from talking to the Prometheus developers themselves - that is how they want it to work. And really, it does make sense

FWIW, the Go client supports non-global registry (see Advanced uses of the registry)

One core Prometheus developer that I know prefers to have his objects implement Collector and register those.

I haven't really thought much about how this would translate into Haskell, although I will need to shortly, as I'd like to do Prometheus instrumentation for a client library I'm writing.

But, ultimately, an equivalent of MustRegister (i.e. panic on invalid metric) into a global registry is the 90% common case.

• the only thing that is more questionable is unsafe registration of metrics.

I sort of started poking into this for prometheus-client—creating an opaque type with a smart constructor that worked as evidence of valid metricness. Required a lot of changes and I didn't get anywhere useful.

Use of String instead of Text

I'm working on switching this out.

Thanks! I also tried this but ran out of energon cubes.

Other wishes from me, in order:

• protobuf / gRPC exporter
• Summary back (I've tried, but cannot understand the maths in the Go equivalent with the resources I have)
• Haskell implementation of Prometheus client API (see https://godoc.org/github.com/prometheus/client_golang/api)

[ocharles]

I think with the registry we can support registering to specific registries, and just have a top level globalRegistry that is used implicitly, or with registerTo globalRegistry.

The prometheus-client does let you have a Metric which is actually lots of metrics - that's how the GHC metric works. http://hackage.haskell.org/package/prometheus-metrics-ghc-0.2.0/docs/Prometheus-Metric-GHC.html is this what you're thinking of when you mean about things have a Collector?

I sort of started poking into this for prometheus-client—creating an opaque type with a smart constructor that worked as evidence of valid metricness. Required a lot of changes and I didn't get anywhere useful.

Do you mean you want something like what prometheus-effect does? To recap, in prometheus-effect, I distinguish between unregistered and registered metrics:

counter :: Metric Counter
incCounter :: Counter -> IO ()
register :: Metric Counter -> IO Counter

I think this is a much safer API, and I've already got a commit changing prometheus-client to use it - it's not much work.

protobuf / gRPC exporter

A protobuf exporter won't be necessary. I actually got into this work by asking in #prometheus if I should use protobufs, and they said no - the text exporter is almost always what you want. The documentation mentioning that it's more efficient is out of date, and in 2.0 the text exporter is really fast. So, unless you want to use the protobuf stuff for something else, we don't need this.

[NicolasT]

Not very Haskelly, but from talking to the Prometheus developers themselves - that is how they want it to work. And really, it does make sense

It may make some sense in this case (though reminds me of singleton pattern, nullary type classes and difficulty to test such things in isolation), but I'm not sure the API and behaviour of Haskell libraries should be dictated by... well... non-Haskell devs or non-Haskell APIs.

To me this is a bit similar to the OpenTracing situation. The spec specifies some API etc, but feels very geared towards a Go/Python/JavaScript-like environment, and as a result doesn't "feel" Haskell-y (at least to me). Hence I have no problem whatsoever to ignore such spec/API and implement what feels right in the target ecosystem.

No built-in way to register the metrics in some Wai app, as far as I could see. http://hackage.haskell.org/package/wai-middleware-prometheus-0.2.0/docs/Network-Wai-Middleware-Prometheus.html

I completely missed that one, thanks for the pointer.

[ocharles]

I started work on making changes - https://github.com/fimad/prometheus-haskell/pull/25. Would love any feedback.

[jml]

Will take a look On Tue, 5 Dec 2017 at 11:41, Oliver Charles notifications@github.com wrote:

I started work on making changes - fimad/prometheus-haskell#25 https://github.com/fimad/prometheus-haskell/pull/25. Would love any feedback.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ocharles/prometheus-effect/issues/6#issuecomment-349279722, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHq6sVofUX4T_ltsT8s3lFwz0NikIA_ks5s9SvKgaJpZM4QhJVI .

[qrilka]

@ocharles in the current code I see modifyIORef' and e.g. in 493ee1902740d6a562dff13118a390c92ae6b2a8 you've removed a couple of atomic operations. In our app we see some weird numbers which look be explained by using modifyIORef' from concurrent threads. And in https://github.com/fimad/prometheus-haskell/pull/25 I see Atomics.atomicModifyIORefCAS_ so am I right that to (easily) get proper atomic counters I need to switch to prometheus-client when that PR will get merged?

[ocharles]

Honestly, I have a really hard time understanding the memory model behind IORefs. In prometheus-client I've left it as is (using atomic-primops). I don't think atomic-primops is strictly necessary, as it still falls back to atomicModifyIORef if a tick count is exhausted (http://hackage.haskell.org/package/atomic-primops-0.8.1/docs/src/Data-Atomics.html#atomicModifyIORefCAS_). I do think that using modifyIORef may be a bug though. Perhaps try my fork of prometheus-client (https://github.com/fimad/prometheus-haskell/pull/25) for an API that's almost identical to this library, and see if the metrics look more plausible?

[qrilka]

Sure I'll check it out

[danclien]

@ocharles, FYI, here's a test repo that replicates Counter losing incCounter calls in prometheus-effect: https://github.com/danclien/prometheus-effect-load-test

I'll give prometheus-client a shot soon.

[danclien]

I checked prometheus-client, and that worked as expected.

Test repo: https://github.com/danclien/prometheus-client-load-test

[ocharles]

Thanks for checking that @danclien! Could you make one more test and see if it works in prometheus-client if you just change https://github.com/fimad/prometheus-haskell/blob/master/prometheus-client/src/Prometheus/Metric/Counter.hs#L38 to just call Data.IORef.atomicModifyIORef? I'm not convinced the atomic primops are necessary.

[danclien]

@ocharles, it seems to work with automicModifyIORef also.

• Branch: https://github.com/danclien/prometheus-client-load-test/tree/no-cas
• Counter.hs
• Using a local copy of prometheus-client that's also checked into the branch

[ocharles]

Excellent, I had a feeling it would, as atomic-primop just falls back to atomicModifyIORef anyway. I don't think we need it, as it just basically provides speculative evaluation, which is a bit overkill for what is literally just + 1.

[qrilka]

@ocharles could you explain a bit your point here? Are you saying that atomic-primops is not needed or that atomicModifyIORef is not needed? If the latter how do you propose to guarantee proper behavior in case of high concurrency? Switching to a TVar? In any as I understand the idea is to switch to prometheus-client which uses atomic-primops, isn't it?

[ocharles]

@qrilka That atomic-primops is not needed. atomicModifyIORef is needed, but atomic-primops just falls back to that anyway. The idea is definitely to switch to prometheus-client, but I'm not convinced it needs atomic-primops, atomicModifyIORef is likely sufficient. See the link in https://github.com/ocharles/prometheus-effect/issues/6#issuecomment-349642445 where I demonstrate that atomicModifyIORefCAS is just atomicModifyIORef with speculative evaluation.

[ocharles]

Given my vague understanding, though, I will probably leave it alone for my initial PR that moves prometheus-effect into prometheus-client.