feat: add playback monitoring #267
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Resolves #262, by allowing to track media playback sessions. With to the data sent, it will be possible to have a better overview of the quality of our service. Finally, it will help us to investigate potential causes of playback problems, so that we can better help our users or provide them with a more accurate response. - rename `analytics` folder to `trackers` - add `PillarboxMonitoring` class - modify `srgssr` middleware to add support for new tracker - add unit test - extract `srg-analytics` player mock to make it reusable - rename `test/analytics` folder to `test/trackers`
|
Coverage report
Show new covered files 🐣
Test suite run success247 tests passing in 10 suites. Report generated by 🧪jest coverage report action from dccfca7 |
jboix
reviewed
Sep 24, 2024
jboix
reviewed
Sep 24, 2024
| * @param {Event} [event] The event that triggered the stop. This is passed | ||
| * to the reset function. | ||
| */ | ||
| sessionStop(event) { |
There was a problem hiding this comment.
It looks like sessionStop can be called multiple times under different conditions and events: loadStart, sessionStart, player dispose, beforeUnload... However, there’s no check in sessionStop to ensure the stop signal is only sent once per session. Maybe we can mitigate this by doing:
sessionStop(event) {
if (this.sessionStopped) return;
// ...
this.sessionStopped = true; // Mark session as stopped
}And then when we send the start event:
loadedData() {
// ...
this.sessionStopped = false;
}
jboix
reviewed
Sep 24, 2024
| * Starts a new session by first stopping the previous session, then resetting | ||
| * the session start timestamp and media ID to their new values. | ||
| */ | ||
| sessionStart() { |
There was a problem hiding this comment.
I would suggest a name here too, I find it inconsistent that:
sessionStop: Sends the stop event and resets the state of the session.sessionStart: Starts tracking the session but doesn't send the start event.
I would suggests something like initSessionTracking or initMetricsTracking.
jboix
reviewed
Sep 24, 2024
| // active, we still want to send the STOP event so that it is properly | ||
| // stopped. | ||
| if ( | ||
| (this.isTrackerDisabled() && !this.currentSessionId) || |
There was a problem hiding this comment.
I feel like a lot of this logic can be simplified and decoupled from this class by introducing a state machine pattern, I would suggest something like this:
class SessionStateMachine {
constructor() {
this.state = 'INITIAL'; // Initial state
}
get isInitial() {
return this.state === 'INITIAL';
}
startSession() {
if (this.isInitial) return false;
this.state = 'SESSION_STARTED';
return true;
}
get isSessionStarted() {
return this.state === 'SESSION_STARTED';
}
endSession() {
if (this.isSessionStarted) return false;
this.state = 'SESSION_ENDED';
return true;
}
errorSession() {
if (this.state !== 'SESSION_STARTED') return false;
this.state = 'SESSION_IN_ERROR';
return true;
}
get isSessionStopped() {
return this.state === 'SESSION_IN_ERROR' ||
this.state === 'SESSION_ENDED';
}
resetSession() {
if (this.isSessionStopped) return false;
this.state = 'INITIAL';
return true;
}
}Then you could simply check the machine state here:
if (!this.state.sessionStarted) return;Resolves #269, sending the number of frame drops during the media playback session. - adds `frame_drops` property - adds the `getVideoPlaybackQuality` function to the player mock
This fix mainly conforms to the latest version of the specification, as some values were not in capital case. - srgssr middleware: rename tracker instance from `srgMonitoring` to `pillarboxMonitoring` - srgssr middleware: start a session directly by calling the `sessionStart` function instead of sending an event - pillarbox-monitoring: delete `pillarbox-monitoring/sessionstart` event - pillarbox-monitoring: stringify error log - srgssr middleware: add test cases for tracker initialization
1 task
|
After a discussion with @jboix, it's not impossible that in a future iteration this class will be extracted into a plugin/component. |
|
🎉 This PR is included in version 1.14.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Resolves #262, by allowing to track media playback sessions. With to the data sent, it will be possible to have a better overview of the quality of our service. Finally, it will help us to investigate potential causes of playback problems, so that we can better help our users or provide them with a more accurate response.
Changes made
analyticsfolder totrackersPillarboxMonitoringclasssrgssrmiddleware to add support for new trackersrg-analyticsplayer mock to make it reusabletest/analyticsfolder totest/trackers