voc / srtrelay

SRT relay server for distributing media streams to multiple clients.
MIT License
134 stars 37 forks source link

Exporting socket stats #8

Closed kevmo314 closed 3 years ago

kevmo314 commented 3 years ago

One feature in SLS that was present was the ability to export the SRT socket's stats. Actually, I'd be willing to contribute this too as the feature seems relatively simple. I'd be happy to send a PR, but wanted to make sure the implementation aligns with what you expect and there isn't any work already being done?

I would implement it as basically for every socket connection, periodically get the stats and POST it to an HTTP endpoint specified in the config.

iSchluff commented 3 years ago

Hi, I think I would prefer the stats to be exposed through the existing http-server. I would either expand https://github.com/voc/srtrelay/blob/dbc02455772e071c9258039a31734302b292d5d6/srt/server.go#L251 for per-stream stats or add an additional endpoint for per-socket stats.

Would that be ok for you or do you have a strict requirement for POSTing the info?

kevmo314 commented 3 years ago

Nope that works for me! I'll send a PR for that.

Only thought I have, I would like to have timeseries statistics to be able to see these stats over the course of an entire stream. I think it doesn't make sense for srtrelay to maintain this as it would result in ever-increasing memory usage? Happy to poll the http-server endpoint, just want to align my thoughts :)

iSchluff commented 3 years ago

Yeah I wouldn't maintain that internally as there are enough timeseries implementations out there. If you are just interested in timeseries it would probably be best to directly expose something like the prometheus text format. I would be fine with using the prometheus library to do that. However I am not sure how detailed the exported data should be. Per socket stats could probably get very large very quickly, so maybe it would make sense to pre-aggregate by senders/receivers for each stream. Depends a bit on what you want to do.

I think it would make sense to start out with a limited set of useful statistics and expand from there.

ravenium commented 3 years ago

There might be an odd panic as a result of this:

srtrelay_1     | github.com/voc/srtrelay/relay.(*Channel).Stats(...)
srtrelay_1     |    /tmp/srtrelay/relay/channel.go:117
srtrelay_1     | github.com/voc/srtrelay/relay.(*RelayImpl).GetStatistics(0xc00021a000, 0x0, 0x0, 0x0)
srtrelay_1     |    /tmp/srtrelay/relay/relay.go:103 +0x3b3
srtrelay_1     | github.com/voc/srtrelay/srt.(*ServerImpl).GetStatistics(0xc000218060, 0xc000290c90, 0x77dd3a, 0xc)
srtrelay_1     |    /tmp/srtrelay/srt/server.go:276 +0x49
srtrelay_1     | github.com/voc/srtrelay/api.(*Server).HandleStreams(0xc0002180c0, 0x7e8580, 0xc0003a2460, 0xc0006ca100)
srtrelay_1     |    /tmp/srtrelay/api/server.go:54 +0x11f
srtrelay_1     | net/http.HandlerFunc.ServeHTTP(0xc00021e020, 0x7e8580, 0xc0003a2460, 0xc0006ca100)
srtrelay_1     |    /usr/lib/go/src/net/http/server.go:2042 +0x44
srtrelay_1     | net/http.(*ServeMux).ServeHTTP(0xc000222040, 0x7e8580, 0xc0003a2460, 0xc0006ca100)
srtrelay_1     |    /usr/lib/go/src/net/http/server.go:2417 +0x1ad
srtrelay_1     | net/http.serverHandler.ServeHTTP(0xc00022a000, 0x7e8580, 0xc0003a2460, 0xc0006ca100)
srtrelay_1     |    /usr/lib/go/src/net/http/server.go:2843 +0xa3
srtrelay_1     | net/http.(*conn).serve(0xc0002340a0, 0x7e8d40, 0xc000223380)
srtrelay_1     |    /usr/lib/go/src/net/http/server.go:1925 +0x8ad
srtrelay_1     | created by net/http.(*Server).Serve
srtrelay_1     |    /usr/lib/go/src/net/http/server.go:2969 +0x36c
srtrelay_1     | 2021/03/26 05:10:38 http: panic serving 172.19.0.2:34352: interface conversion: interface {} is nil, not int
srtrelay_1     | goroutine 108665 [running]:
srtrelay_1     | net/http.(*conn).serve.func1(0xc0002341e0)
srtrelay_1     |    /usr/lib/go/src/net/http/server.go:1801 +0x147

I'm going to have to do a little more digging but that cropped up after a while of running it in docker.

iSchluff commented 3 years ago

I think the atomic.Value has to be initialized on NewChannel, otherwise there is probably a race condition between pub and stats.

So not related to your PR, but related to stats.

iSchluff commented 3 years ago

Should be fixed with 4c55c804d5100a2e34b424404c0cf25e81903863