prometheus-junkyard / mesos_exporter

Prometheus exporter for Mesos, deprecated.
Apache License 2.0
42 stars 22 forks source link

Slave discovery #5

Closed tommyulfsparre closed 9 years ago

tommyulfsparre commented 9 years ago

This PR is just a suggestion for some changes to the mesos_exporter, namely:

this I believe also fixes #2 , but also means if the exporter fails to fetch a slaves metrics it will disappear until it can be reached again.

Do these changes sound reasonable @juliusv @antonlindstrom ?

and thanks for writing this @antonlindstrom !

juliusv commented 9 years ago

@tommyulfsparre Thanks for sending this in! We don't have Mesos experience ourselves, so I'm hoping for @antonlindstrom to give his opinion here on the general concept.

Happy to help review the code itself later on, but can't really help with Mesos-specific things.

brian-brazil commented 9 years ago

From an architectural standpoint I wonder if this would be better done as a mesos_exporter per mesos slave, and then using some service discovery mechanism to find them all on the prometheus server.

tommyulfsparre commented 9 years ago

@brian-brazil just curious, how is that better than outsourcing the collection/discovery to the mesos_exporter? What is the benefits?

The prometheus server might already have a bunch of targets to scrape and the only service discovery you can do ATM is via SRV records, right?

antonlindstrom commented 9 years ago

@tommyulfsparre I think this looks really nice! I like that you've limited the concurrent requests.

One thing that concerns me is that the performance of the exporter once you have a huge number of slaves and tasks. What do you think about adding a way to connect to a single slave as well? That way, it's possible to run an exporter on every node as @brian-brazil suggests.

I also think the Dockerfile has to change to use the flags instead of the config as well.

Otherwise, I think this is super! Well done, @tommyulfsparre!

tommyulfsparre commented 9 years ago

@antonlindstrom thanks!

That only shifts the burden to the prometheus server instead, you only amortize the overhead of fetching and deserialising the statistics.json I guess.

Anyway small change to support both modes, so I added it.

brian-brazil commented 9 years ago

@tommyulfsparre Spreads the load better, and is easier to manage as you remove a SPOF.

tommyulfsparre commented 9 years ago

@brian-brazil fair enough, but I would assume that the mesos_exporter would be run as a task on Mesos.

@juliusv @antonlindstrom I think this is ready to be reviewed now :)

antonlindstrom commented 9 years ago

@tommyulfsparre LGTM! Nice work! :bow:

tommyulfsparre commented 9 years ago

@juliusv thanks for the review! Fixed and pushed

discordianfish commented 9 years ago

This is awesome! LGTM.

@tommyulfsparre: Does this mean Battlefield is now powered by Prometheus? :)

tommyulfsparre commented 9 years ago

@discordianfish this is something I'm toying with on my spare time

@juliusv address -> url

discordianfish commented 9 years ago

@tommyulfsparre Aww, too bad. Hope you'll toy with it at work sometime soon :)

juliusv commented 9 years ago

:+1: @tommyulfsparre Woot, thanks for bearing with me all this time! Merging!