ai/live: Send event when auth fails #3331
Conversation
j0sh
commented
Dec 19, 2024
j0sh
commented
- Send an event when auth fails. If auth is broken, we don't get any events, so it can be hard to determine from events what the state of things is.
- Set the streamID = streamKey by default. This is helpful in dev mode without a webhook setup. If a webhook specifies streamID then that is still used.
- Update some logging
j0sh
requested review from
victorges,
leszko,
mjh1,
ecmulli and
gioelecerati
github-actions
bot
added
go
| @@ -454,6 +455,10 @@ func (ls *LivepeerServer) StartLiveVideo() http.Handler { | |||
| clog.Errorf(ctx, "failed to kick input connection: %s", kickErr.Error()) | |||
| } | |||
| clog.Errorf(ctx, "Live AI auth failed: %s", err.Error()) | |||
| monitor.SendQueueEventAsync("ai_stream_events", map[string]string{ | |||
| "type": "error", | |||
| "message": "Live AI auth failed: " + err.Error(), | |||
There was a problem hiding this comment.
@ecmulli @gioelecerati wanted to check this would not break anything downstream since we only have a limited set of fields here. Most of the rest are unknown at this point - they come as part of the auth webhook, but the auth webhook has failed here.
There was a problem hiding this comment.
Ping on this @ecmulli @gioelecerati
There was a problem hiding this comment.
@j0sh this shouldn't break anything, since the messages are sent to Kafka without any deserialization - worst case scenario they won't be collected anywhere
There was a problem hiding this comment.
@j0sh, @gioelecerati is correct here. the missing fields should be ignored.
is streamId, pipelineID, etc automatically included in this?
There was a problem hiding this comment.
is streamId, pipelineID, etc automatically included in this?
They are not included, because they don't exist at this point. We typically receive those via the auth webhook but this event is reporting that the auth webhook has failed.
If we wanted some form of identifier we could include the stream key as stream_key or something, although that is not part of other events IIRC. The key itself might also be a little strange from brute force connection attempts which has some risk to downstream consumers through eg, injection.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3331 +/- ##
============================================
Coverage ? 33.75090%
============================================
Files ? 141
Lines ? 37359
Branches ? 0
============================================
Hits ? 12609
Misses ? 24030
Partials ? 720
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Looks good, but let's wait for @ecmulli @gioelecerati before merging to confirm/answer your inline question
