palavatv / signaltower

Signaling server for WebRTC video-/audio conferencing using the palava protocol
https://palava.tv
GNU General Public License v3.0
39 stars 7 forks source link

Feature/erikzenker/prometheus interface #2

Closed erikzenker closed 4 years ago

erikzenker commented 4 years ago

This PR add prometheus interface to the signaltower under the route /metrics. Currently it exposes palava_leave_room_total, palava_joined_room_total, and erlang_vm metrics

farao commented 4 years ago

Thanks! Some feedback: 1) I just realised that we should not open the /metrics route to the whole network, but only to localhost. I thought I could fix this by using the following routes config but it doesn't open /metrics at all :-/ not quite sure why it's not working:

dispatch = :cowboy_router.compile([
      {:_,          [{"/", SignalTower.WebsocketHandler, []}]},
      {"localhost", [{"/metrics", SignalTower.PrometheusHTTPHandler, []}]}
])
  1. it would be good if we could somehow completely replace the current file based stats (but have the functionality of logging anonymously conversations with start and end date and maximum number of participants when they're done. This could probably replace the join and leave counters? This would mean also that we delete the Stats module when the prometheus server can take over this part.
  2. the question about the to_string function
farao commented 4 years ago

Ah, I found a solution to point 1: https://github.com/farao/signaltower/commit/f48bcb2385f7c2286855154fac775529b2c49b33 (the order was incorrect in my comment). Please add this commit to the pull request

erikzenker commented 4 years ago
1. it would be good if we could somehow completely replace the current file based stats (but have the functionality of logging anonymously conversations with start and end date and maximum number of participants when they're done. This could probably replace the join and leave counters? This would mean also that we delete the Stats module when the prometheus server can take over this part.

I would increase the number of metrics instead of replacing them.

But lets do it in followups.

What is the purpose of the Stats module? We should not touch it with this PR, but might do something with it in a followup.

erikzenker commented 4 years ago

Ah, I found a solution to point 1: f48bcb2 (the order was incorrect in my comment). Please add this commit to the pull request

Is the port directly exposed public by cowboy or is there some webserver (nginx) in front of it? On which port does the signaltower listening?

farao commented 4 years ago

Ok, then let's do the added functionality in follow ups and also remove the stats module as a follow up when it's functionality is subsumed by the prometheus module. The stats module until now logged these information in a non standard way in a plain file - the prometheus solution should be the only one so that we don't need to maintain two stats modules.

For the port: on the palava server it's behind an nginx, but I would like to make it default that metrics only get offered internally if somebody runs an own palava server. And if somebody wants to run a palava server and expose stats externally, this would also be possible via nginx or another reverse proxy. The port is btw configurable.