Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updated the metrics reporting url #36

Merged
merged 8 commits into from
Sep 8, 2022
Merged

Updated the metrics reporting url #36

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5205d64
Added more tests
Sep 8, 2022
afae228
Pointing to the new reporting base url
Sep 8, 2022
5adb666
Added more tests
Sep 8, 2022
8fe9120
Updated the base URL for the metrics reporting
Sep 8, 2022
7f43012
Added a console warning when not able to report metrics
Sep 8, 2022
28a4997
Merge branch 'main' into clacladev/metrics-reporting-url
0xcadams Sep 8, 2022
0521877
fix: update URL to point to playback.livepeer.fun
0xcadams Sep 8, 2022
c60c3e6
test: fix tests
0xcadams Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/core/src/video/hls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,14 @@ export const createNewHls = (
}
});

// TODO: re-enable after testing and before merging
const metricReportingUrl = createMetricsReportingUrl(source);
if (metricReportingUrl) {
reportVideoMetrics(element, metricReportingUrl);
} else {
console.log(
'Not able to report player metrics given the source url',
source,
);
}
});

Expand Down
62 changes: 54 additions & 8 deletions packages/core/src/video/metrics/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,59 @@ import { describe, expect, it } from 'vitest';
import { createMetricsReportingUrl } from './utils';

describe('createMetricsReportingUrl', () => {
it('creates', () => {
const url = createMetricsReportingUrl(
'https://livepeercdn.com/recordings/c34af47b-bbf2-40ed-ad2d-77abd43860c9/index.m3u8',
);

expect(url).toMatchInlineSnapshot(
'"wss://sao-canary-catalyst-0.livepeer.fun/json_video+c34af47b-bbf2-40ed-ad2d-77abd43860c9.js"',
);
describe('asset url', () => {
it('given a valid url then it should return a reporting url', () => {
// Given
const sourceUrl =
'https://livepeercdn.com/hls/172159gos7h0pq17/index.m3u8';
// When
const reportingUrl = createMetricsReportingUrl(sourceUrl);
// Then
expect(reportingUrl).toEqual(
'wss://nyc-canary-catalyst-0.livepeer.fun/json_video+172159gos7h0pq17.js',
);
});

it('given invalid urls then it should not return a reporting urls', () => {
// Given
const sourceUrls = [
'https://livepeercdn.com/dash/172159gos7h0pq17/index.m3u8',
'https://livepeercdn.com/hls/172159gos7h0pq17/master.m3u8',
];
// When
const reportingUrls = sourceUrls.map((url) =>
createMetricsReportingUrl(url),
);
// Then
expect(reportingUrls).toEqual([undefined, undefined]);
});
});

describe('recording url', () => {
it('given a valid url then it should return a reporting url', () => {
// Given
const sourceUrl =
'https://livepeercdn.com/recordings/c34af47b-bbf2-40ed-ad2d-77abd43860c9/index.m3u8';
// When
const reportingUrl = createMetricsReportingUrl(sourceUrl);
// Then
expect(reportingUrl).toEqual(
'wss://nyc-canary-catalyst-0.livepeer.fun/json_video+c34af47b-bbf2-40ed-ad2d-77abd43860c9.js',
);
});

it('given invalid urls then it should not return a reporting urls', () => {
// 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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

];
// When
const reportingUrls = sourceUrls.map((url) =>
createMetricsReportingUrl(url),
);
// Then
expect(reportingUrls).toEqual([undefined, undefined]);
});
});
});
28 changes: 21 additions & 7 deletions packages/core/src/video/metrics/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victorges Are you able to clarify? Is livepeer.fun another staging environment or is it production?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding from the conversation is that .studio does not exists yet. It will eventually.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const METRICS_REPORTING_BASE_URL = 'wss://nyc-canary-catalyst-0.livepeer.fun';

function videoNameFromPlaybackUrl(url: string): string | undefined {
const filename: string[] = url.split('/');
const videoName = filename[filename.length - 2];
return videoName ? videoName.split('.')[0] : undefined;
const PLAYLIST_NAME = 'index.m3u8';
const ASSET_URL_PART_VALUE = 'hls';
const RECORDING_URL_PART_VALUE = 'recordings';

// Example url the playback id needs to be found in
// https://livepeercdn.com/hls/<playback-id>/index.m3u8
// https://livepeercdn.com/recordings/<playback-id>/index.m3u8
function playbackIdFromPlaybackUrl(url: string): string | undefined {
const parts = url.split('/');
const playlistPartIndex = parts.indexOf(PLAYLIST_NAME);
const assetPartIndex = parts.indexOf(ASSET_URL_PART_VALUE);
const recordingPartIndex = parts.indexOf(RECORDING_URL_PART_VALUE);

// Check if the url is valid
return (assetPartIndex !== -1 || recordingPartIndex !== -1) &&
playlistPartIndex !== -1
? parts[playlistPartIndex - 1]
: undefined;
}

export function createMetricsReportingUrl(src: string): string | undefined {
const videoName = videoNameFromPlaybackUrl(src);
const videoName = playbackIdFromPlaybackUrl(src);
return videoName
? `${METRICS_REPORTING_BASE_URL}/json_video+${videoName}.js`
: undefined;
Expand Down