-
Notifications
You must be signed in to change notification settings - Fork 170
Feature: Dump latency stats to CSV | Re-ship: 4.27 browser send offer support #647
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.
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
Merged
Conversation
This file contains hidden or 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
* feat: Adding script tests where the scripts are run multiple times. This tests that the scripts still work when things have already been installed. * fix: Fixing typo in github action. * gimme a break. * Adding mutliple script runs for other OSs * Testing node on the runner * Trying to figure out what node is doing what. * Trying to change the way node version is detected. * Messy * The additions to the script really didn't catch anything. Could be worth revisiting how we handle the local node install. * Removing debug print. (cherry picked from commit 2321fa9)
[LatencyTest] Fixing more path issues in platform_scripts (#604)
Updated the way the bash script sets up the environment for node. Node will only be installed if a version meeting the minimum requirements is not found. The new environment should result in a more robust environment setup. (cherry picked from commit f27bace)
[LatencyTest] refactor: Better handling of node in Linux scripts. (#607)
If the node install was installed locally previously the script would not pick that up and try to install again. It would fail because the directory already exists and then the path would not get properly setup for the local install of node. This change sets PATH to point to the local install anyway. If it doesn't exist it doesn't matter, if it does exist we use the local install. (cherry picked from commit 00aaa52)
[LatencyTest] Fixing a small issue with how path was setup previously. (#611)
🦋 Changeset detectedLatest commit: bf5ee5a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
mcottontensor
approved these changes
May 29, 2025
github-actions bot
added a commit
that referenced
this pull request
May 29, 2025
… support (#647) * Add 4.27 support back for latency testing * Save webrtc stats to csv and auto download * Deep copy stats object * Update record parsing logic to handle missing field when generating the csv * Update session test to handle additional latency stats * Fixing path setup in the windows platform scripts. (#594) (#595) (cherry picked from commit 152abd6) Co-authored-by: mcottontensor <80377552+mcottontensor@users.noreply.github.com> (cherry picked from commit 1dedc87) * Fixing more path issues in platform_scripts (#604) * feat: Adding script tests where the scripts are run multiple times. This tests that the scripts still work when things have already been installed. * fix: Fixing typo in github action. * gimme a break. * Adding mutliple script runs for other OSs * Testing node on the runner * Trying to figure out what node is doing what. * Trying to change the way node version is detected. * Messy * The additions to the script really didn't catch anything. Could be worth revisiting how we handle the local node install. * Removing debug print. (cherry picked from commit 2321fa9) * refactor: Better handling of node in Linux scripts. (#607) Updated the way the bash script sets up the environment for node. Node will only be installed if a version meeting the minimum requirements is not found. The new environment should result in a more robust environment setup. (cherry picked from commit f27bace) * Fixing a small issue with how path was setup previously. (#611) If the node install was installed locally previously the script would not pick that up and try to install again. It would fail because the directory already exists and then the path would not get properly setup for the local install of node. This change sets PATH to point to the local install anyway. If it doesn't exist it doesn't matter, if it does exist we use the local install. (cherry picked from commit 00aaa52) * Added disable button when session test is running * Added new flag ?LatencyCSV to enable the session test and its UI * Added changeset for 4.27 support and session test feature --------- Co-authored-by: Will Belcher <william.belcher@xa.epicgames.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: mcottontensor <80377552+mcottontensor@users.noreply.github.com> (cherry picked from commit c61fd7b)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
mcottontensor
added a commit
that referenced
this pull request
May 29, 2025
… support (#647) (#651) * Add 4.27 support back for latency testing * Save webrtc stats to csv and auto download * Deep copy stats object * Update record parsing logic to handle missing field when generating the csv * Update session test to handle additional latency stats * Fixing path setup in the windows platform scripts. (#594) (#595) (cherry picked from commit 152abd6) (cherry picked from commit 1dedc87) * Fixing more path issues in platform_scripts (#604) * feat: Adding script tests where the scripts are run multiple times. This tests that the scripts still work when things have already been installed. * fix: Fixing typo in github action. * gimme a break. * Adding mutliple script runs for other OSs * Testing node on the runner * Trying to figure out what node is doing what. * Trying to change the way node version is detected. * Messy * The additions to the script really didn't catch anything. Could be worth revisiting how we handle the local node install. * Removing debug print. (cherry picked from commit 2321fa9) * refactor: Better handling of node in Linux scripts. (#607) Updated the way the bash script sets up the environment for node. Node will only be installed if a version meeting the minimum requirements is not found. The new environment should result in a more robust environment setup. (cherry picked from commit f27bace) * Fixing a small issue with how path was setup previously. (#611) If the node install was installed locally previously the script would not pick that up and try to install again. It would fail because the directory already exists and then the path would not get properly setup for the local install of node. This change sets PATH to point to the local install anyway. If it doesn't exist it doesn't matter, if it does exist we use the local install. (cherry picked from commit 00aaa52) * Added disable button when session test is running * Added new flag ?LatencyCSV to enable the session test and its UI * Added changeset for 4.27 support and session test feature --------- (cherry picked from commit c61fd7b) Co-authored-by: Luke Bermingham <1215582+lukehb@users.noreply.github.com> Co-authored-by: Will Belcher <william.belcher@xa.epicgames.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: mcottontensor <80377552+mcottontensor@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
auto-backport
Used to specify we want a PR to auto backport to a branch, must be paired with auto-backport-to-UEX.
auto-backport-to-UE5.6
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.
Relevant components:
Problem statement:
Adds a new feature to run a variable length latency test session (e.g. a 60s window) and dump that stats from the session to two
.csvfiles:latency.csv- Which contains the video timing statsstats.csv- Which contains all WebRTC stats the library currently tracksAdditionally: It was found useful to support running this test against UE 4.27 for internal historical testing, so support for connecting to this version has been restored.
Solution
Frontend\ui-library\src\UI\SessionTest.ts?LatencyCSVthat enables this feature (by default it is disabled)?BrowserSendOfferwhich makes the browser send the WebRTC offer (this should only be used with 4.27 projects).Documentation
This feature is for advanced use and internal testing only so both the
?LatencyCSVand?BrowserSendOfferflags are disabled by default.To use this new feature:
http://localhost/?LatencyCSV.Run Testunder the Session Test heading.Optional for 4.27 projects: Add
?BrowserSendOfferto the url parameters or set it programmatically.Test Plan and Compatibility
.csvfiles with latency stats.