Fix stat_post_url functionality (Fixes #43) #54
Conversation
|
hi, odensc |
|
Can confirm this is working, tested with docker and a couple of independent streams. FWIW it gets my thumbs up for a merge :) Couple thoughts: @Edward-Wu Is the fact that on_event is a bunch of URL parameters and stats is a json payload by design? It might make parsing more sane if they were both json POST bodies. |
|
2 more things:
|
|
@ravenium on_event seemed to work in my limited testing.. Can you share the logs from around the time when you close the stream? Wondering if the HTTP request is being attempted and just failed, or if SLS even detected that the stream closed at all. (also I just remembered your name, I have used your docker container for a few things :) |
|
@odensc Awesome! :) I haven't been able to get our streaming group to give it a whirl until they get a way of receiving notifications. Well, that and they like RTMP's ability to preview in a browser. Bah! I did a quick start, count to 5, stop with on_event turned on. Naturally the first time I tried, it worked for both on_connect and on_close, but here's one where it didn't: Here's the event log when I clicked "stop streaming" in OBS:
It looks like the second generated http request fails in some way. I strapped a pcap to the test web server I'm using and it (correctly) sees the entire on_connect exchange, but on_close only gets as far as a SYN/SYNACK, at which point it RSTs. |
|
I spun up a new version of SLS this week and I'm not receiving the on_close call. I'm going to revert to an old version, as this is a critical call to our management service. |
|
@fullbrightness do you know what version it broke on? or are you saying this PR broke it? |
|
@odensc I compiled early this week, but I didn't include this pull request, so probably the main version. The other instance I have running I compiled in June. I read your post before realizing this hadn't been merged back in yet. |
|
[0x7fd29034c090]CHttpClient::generate_http_request, ok, m_url='http://:<>/sls/on_event?on_event=on_close&role_name=publisher&srt_url=video.tx/live/streamname-mv&remote_ip=<>&remote_port=59310', content len=0. I see the event being generated but its not actually happening. The on_connect works every time. The version thats not working is actually running on Ubuntu 18.04 whereas the the version that is working is running in the cloud on 20.04. |
That debug log looks awfully close to mine - I'm not a C++ guy (Golang is my crutch) but it looks like the request gets reset before it can even handshake. |
|
Seems like some sort of race condition possibly. I don't currently have the time to look into this in depth, as it's not a feature I require. You could add a breakpoint/log to each step in HttpClient.cpp and see where it gets tripped up. Since generate_http_request is being logged with "ok," that means the next step is |
| { | ||
| info_str += '['; |
There was a problem hiding this comment.
Great job debugging and fixing this!
JSON response best practice is to always return a JSON object, not just an Array. I recommend that when sending this data in the stats request body, it be wrapped in an object like { workers: [...workers] }. This way, you can extend the data returned later without making breaking changes.
For reference, this is what I'm seeing in the request body:
[
{
port: '8080',
role: 'player',
pub_domain_app: 'input/live',
stream_name: 'roger',
url: 'output/live/roger',
remote_ip: '192.168.1.114',
remote_port: '61426',
start_time: '2020-11-03 11:35:37',
kbitrate: '11033'
},
{
port: '8080',
role: 'publisher',
pub_domain_app: 'input/live',
stream_name: 'roger',
url: 'input/live/roger',
remote_ip: '192.168.1.114',
remote_port: '64291',
start_time: '2020-11-03 11:35:27',
kbitrate: '11765'
},
{
port: '8080',
role: 'listener',
pub_domain_app: '',
stream_name: '',
url: '',
remote_ip: '',
remote_port: '',
start_time: '2020-11-03 11:35:08',
kbitrate: '0'
}
]
There was a problem hiding this comment.
Also, at least for me the response now starts with "[,{ port: ..." in which the comma of course breaks any JSON parsing. This happens when the first worker in the list actually has an empty info. Great direction though, really waiting for these changes to get merged!
There was a problem hiding this comment.
I don't expect any pull requests to be merged. It would be great if someone interested in maintaining the project would fork it, because there are a lot of very small improvements that would make it much more flexible and usable in different scenarios. I've personally moved on to a different solution for SRT streaming, but this project was a great help to me initially.
There was a problem hiding this comment.
Agreed, this project was one of the first I noticed and really got me going for open source SRT streaming. People get busy and such, so it's understandable that not everyone has time to keep up with their stuff. Luckily, it's inspired others to do their own versions - if it's ok to mention other projects here, I've been following voc/srtrelay (which is based in part on sls). The author wrote it in golang so a little more merciful to my coding wheelhouse.
I fixed a few things here to generally bring the stat post functionality up to snuff.
application/jsoncontent-lengthand the request would fail.As a side note, the indentation and formatting of the repo seemed to be all over the place... so I hope I got that right. Also I don't use C++ much, so hopefully some more experienced eyes can take a look here.
Fixes #43.