Only send gateway_ingest_stream_closed on client disconnects

[mjh1]

Instead of firing gateway_ingest_stream_closed event on every type of shutdown we can detect the ICE connection closed state (which happens on client disconnects) and only fire it in those cases as I believe was the intent, right @victorges ?
(I saw an example from prod where we had the ICE connection failed state and we still fired gateway_ingest_stream_closed)

[victorges]

LGTM either way

Instead of firing gateway_ingest_stream_closed event on every type of shutdown we can detect the ICE connection closed state (which happens on client disconnects) and only fire it in those cases as I believe was the intent, right @victorges ?

That was not my intent. I just wanted to create an event that was fired when the ingest stopped, but maybe I picked a bad name for it. I think @ecmulli should make the final decision here since he's the one processing these. I'm perfectly fine only sending these events on "happy" closures, not on the error ones.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.

Project coverage is 30.40271%. Comparing base (13aa3b7) to head (00fdeef).

Files with missing lines	Patch %	Lines
server/ai_mediaserver.go	0.00000%	15 Missing ⚠️
media/whip_server.go	0.00000%	1 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##              master       #3485         +/-   ##
===================================================
- Coverage   30.40631%   30.40271%   -0.00360%     
===================================================
  Files            151         151                 
  Lines          44596       44598          +2     
===================================================
- Hits           13560       13559          -1     
- Misses         30246       30249          +3     
  Partials         790         790                 

Files with missing lines	Coverage Δ
media/whip_server.go	0.00000% <0.00000%> (ø)
server/ai_mediaserver.go	5.28846% <0.00000%> (-0.01274%)	⬇️

... and 1 file with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13aa3b7...00fdeef. Read the comment docs.

Files with missing lines	Coverage Δ
media/whip_server.go	0.00000% <0.00000%> (ø)
server/ai_mediaserver.go	5.28846% <0.00000%> (-0.01274%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mjh1]

LGTM either way

Instead of firing gateway_ingest_stream_closed event on every type of shutdown we can detect the ICE connection closed state (which happens on client disconnects) and only fire it in those cases as I believe was the intent, right @victorges ?

That was not my intent. I just wanted to create an event that was fired when the ingest stopped, but maybe I picked a bad name for it. I think @ecmulli should make the final decision here since he's the one processing these. I'm perfectly fine only sending these events on "happy" closures, not on the error ones.

Ah ok, maybe I should make a different event for this then not sure. I know it'll be hard to implement the same thing for the mediamtx route (but hopefully that's moot if we're not going to be using that again)

[victorges]

Hmm maybe we could have an error field in this event? Not sure if possible

[ecmulli]

LGTM either way

Instead of firing gateway_ingest_stream_closed event on every type of shutdown we can detect the ICE connection closed state (which happens on client disconnects) and only fire it in those cases as I believe was the intent, right @victorges ?

That was not my intent. I just wanted to create an event that was fired when the ingest stopped, but maybe I picked a bad name for it. I think @ecmulli should make the final decision here since he's the one processing these. I'm perfectly fine only sending these events on "happy" closures, not on the error ones.

@victorges in our analysis we've realized that we really need a way to understand if the closure is "happy" or "sad". could we identify that and send an event accordingly? i.e. gateway_crashed vs gateway_successful_close or sth?

[victorges]

If the gateway itself closes the connection will we get to this state as well? We might wanna differentiate the cases where the user happily closes it or the server does it for some reason (like errors)

[victorges]

Got it @ecmulli ! Also talking to @emranemran he also wanted that info more clearly. I guess this is a good to go for this change @mjh1 , just made another comment above to make sure this event will actually mean what we want.

Also, do we need to fix the event when mediamtx is used as well? Or just WHIP is fine from now one

[emranemran]

I think the changes here are ok to go ahead but agree with Evan/Victor above - ideally we can get to a place where we can differenctiate between good/bad disconnect from user and gateway's perspective. I'm guessing it's hard to know from the user's perspective w/o events on the app but at-least on the gateway, could we differentiate between happy/sad disconnect?

[j0sh]

One way to identify a happy vs sad closure right now might be to look at the errors for a stream itself, especially the first error. eg #3480 ("whip disconnected") should indicate a happy closure

As far as this PR goes, it seems useful to report an event whenever a client disconnects, even if there was an error that caused the disconnect. No real opinion on whether that should be a different event for happy vs sad disconnects, whatever works best for @ecmulli

edit: on second thought, I have a slight preference for having the same event be reported instead of specializing the events based on errors, because otherwise we are down the path of reporting a separate event type for each error type, but we already report errors...

[j0sh]

Closing the loop here - from our discussions on Discord, I believe the consensus is to fire gateway_ingest_stream_closed on every disconnect to as an indicator of the stream state. @ecmulli please correct me if I'm wrong.