Updated the metrics reporting url #36
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
LGTM! Just some clarifying questions but looks great.
Also, in the future, try using expect(url).toMatchInlineSnapshot('...') - when you run the tests locally and the snapshot is out of date, it makes it easy to update (press u and the value is automatically updated). Makes it a bit easier to maintain tests.
| @@ -1,14 +1,28 @@ | |||
| // Temporarily hardcoded catalyst node | |||
| const METRICS_REPORTING_BASE_URL = 'wss://sao-canary-catalyst-0.livepeer.fun'; | |||
| // Temporarily hardcoded catalyst node, in production we need to point to .studio | |||
There was a problem hiding this comment.
Should this be pointing to .studio then? Or is this waiting on something? The discussion was a bit confusing to me. This library should only be used in "production" or very similar, production-like environments.
There was a problem hiding this comment.
@victorges Are you able to clarify? Is livepeer.fun another staging environment or is it production?
There was a problem hiding this comment.
My understanding from the conversation is that .studio does not exists yet. It will eventually.
There was a problem hiding this comment.
Reporting URL should have the same TLD as the playback URL. If playback is from .studio, use studio, etc.
Canary (.fun) is a second staging environment created for multi-node catalyst development. Regular staging is under .monster.
When multi-node goes to production, we will have studio, monster and fun URLs.
.fun is the closest we will have to that, since it is the only multi-node deployment we have right now. Although, the SDK here could just keep the same TLD anyway (maybe with a list of allowed ones) and that should be future-proof.
There was a problem hiding this comment.
We could already implement this to keep the same tld as the sourceUrl, but if they are not live yet, then we would not be actually be reporting any metrics right?
There was a problem hiding this comment.
Right! But we cannot deploy/use this on any environment before multi-node is deployed (which is what enables this API). Unless we want to release a version that only works on the canary environment anyway. We cannot use metrics from canary (fun) for production (studio) streams anyway, it's a separate timeseries database. So sending them to canary or not sending at all (like trying to send to playback.livepeer.studio) does not make any difference in practice.
| // Given | ||
| const sourceUrls = [ | ||
| 'https://livepeercdn.com/static/c34af47b-bbf2-40ed-ad2d-77abd43860c9/index.m3u8', | ||
| 'https://livepeercdn.com/recordings/c34af47b-bbf2-40ed-ad2d-77abd43860c9/master.m3u8', |
There was a problem hiding this comment.
Should we deliberately not handle this case? I am not sure what the playlist names look like, or why we'd want to ignore master over index.
There was a problem hiding this comment.
My thought is that if this is not an agreed livepeer url, we end processing some random string as playbackID. Then we try to connect a WS to an incorrect URL. So it's going to fail anyway.
If we want it to be more elastic, I can just check that the last parameter contains .m3u8 and not the specific playlist name.
What are your thoughts?
There was a problem hiding this comment.
IMO we should ignore anything that doesn't look like a Livepeer URL. We don't want to end up sending metrics about non-existing streams when people use the player for non-Livepeer resources (which we should support).
|
Added #38 to track future work on productionizing metrics reporting. |
|
Thanks Chase to get this merged |
Description
Updated the metrics reporting url for the video player.
Additional Information