New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify comment about "native solution" long term in Readme #125

Closed
SamSaffron opened this Issue Feb 7, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@SamSaffron
Copy link

SamSaffron commented Feb 7, 2018

Readme says:

We recommend this only as an intermediate solution and recommend switching to native Prometheus instrumentation in the long term.

I would like this somewhat clarified. For forking web servers such as Passenger, Unicorn and Puma in the Ruby ecosystem, requests are round robined between process forks. This means you have no simple way of sharing state between forks and thus have a hard time collating data for an exporter.

In this kind of setup a solution like statsd_exporter seems reasonable as the alternatives are all similarly messy. Be it sharing a pipe between all forks or nmaping a file and so on.

I think the Readme should clarify that this is a completely legitimate solution for specific use cases such as aggregation of data from forked processes that all share an HTTP listener.

@matthiasr

This comment has been minimized.

Copy link
Contributor

matthiasr commented Feb 7, 2018

I think @SuperQ was working on improving the Ruby client so that statsd is no longer necessary there?

@SuperQ

This comment has been minimized.

Copy link
Member

SuperQ commented Feb 7, 2018

Yes, we have a working mmap-based multiproc Ruby client gem. It needs a little more testing and to be merged upstream.

@matthiasr

This comment has been minimized.

Copy link
Contributor

matthiasr commented Feb 7, 2018

@SamSaffron in light of that, I would like to keep the README as it is – it still is a temporary solution, even if it's been a long temporary.

@matthiasr matthiasr closed this Feb 7, 2018

@SamSaffron

This comment has been minimized.

Copy link

SamSaffron commented Feb 7, 2018

Keep in mind this mmap solution has some serious limitations which is why it has not been merged, especially in pathologic cases where forked processes are crashing.

We ended up building https://github.com/discourse/prometheus_exporter but essentially it ends up being a solution that is similar enough to statsd_exporter that I just don't feel it is justified saying "temporary" for statsd exporter. Sure prom exporter is more flexible, but statsd exporter is also fine, and if implemented somewhere I would not be knocking on anyones door to migrate to ruby prom exporter.

There are other forking servers out there that are not Ruby and forwarding metrics to a central collector on the box is a reasonable solution.

@SuperQ

This comment has been minimized.

Copy link
Member

SuperQ commented Feb 7, 2018

Our mmap solution does not suffer from those problems. The reason it hasn't been merged are unrelated.

@SamSaffron

This comment has been minimized.

Copy link

SamSaffron commented Feb 7, 2018

@SuperQ Good to hear but there are going to be certain levels of pain especially since this is not something that ships with Ruby, so you need a native extension, it will obviously not work in Windows and jRuby is going to need special care.

Originally I built my exporter with IO.pipe which got shared via master process, but as time went by the isolation of having the collection in a dedicated process just became far more appealing then having these metrics floating around in my application server.

@matthiasr

This comment has been minimized.

Copy link
Contributor

matthiasr commented Feb 7, 2018

@SamSaffron

This comment has been minimized.

Copy link

SamSaffron commented Feb 7, 2018

I think that is a totally legit point a collector that was designed from the ground up without needing to worry about statsd quirks would be better. You could batch metrics better, avoid the messy regex and other bonuses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment