feat: RPC Event Net stat reporting - #954
Merged
Rosme merged 3 commits intoJul 14, 2021
Merged
Conversation
Comment on lines
+102
to
+106
|
|
||
| if (NetworkManager.__rpc_name_table.TryGetValue(rpcMethodId, out var rpcMethodName)) | ||
| { | ||
| NetworkManager.NetworkMetrics.TrackRpcSent(NetworkManager.ServerClientId, NetworkObjectId, rpcMethodName, (ulong)serializer.Writer.GetStream().Length); | ||
| } |
Contributor
There was a problem hiding this comment.
Are these the changes that are already in develop?
Author
There was a problem hiding this comment.
What do you mean? There are no changes in develop
Contributor
There was a problem hiding this comment.
Oh, oops. I misinterpreted what this was
| { | ||
| if (!m_NetworkGameObjects.ContainsKey(networkObjectId)) | ||
| { | ||
| m_NetworkGameObjects[networkObjectId] = new NetworkObjectIdentifier($"NetworkGameObject_{networkObjectId}", networkObjectId); |
Contributor
There was a problem hiding this comment.
I'm confused about the name here. Why have a name if it's just hard coded to the network id?
There was a problem hiding this comment.
This looks slightly different to the other metrics, here we're updating two metrics, one is tracking network objects (which if this is something we should do I'll need to make changes to the network object spawned/destroyed metrics as these are just in their own metric type), and one is tracking the RPCs. In the other "Track" functions we're only updating one metric type.
Author
There was a problem hiding this comment.
My idea here is that if for some reason we do not have the GameObject registered yet, we could create it on the fly and have it's name be the id. I don't mind removing that though.
Contributor
There was a problem hiding this comment.
Since it allocates a runtime string I'd say it isn't worth it. I'd rather put a literal like "Unknown" for this case
| { | ||
| MultiInstanceHelpers.Destroy(); | ||
|
|
||
| yield return null; |
Contributor
There was a problem hiding this comment.
Does this actually need to yield or could it just be a regular [Teardown]?
Author
There was a problem hiding this comment.
I used what was done for the NetworkVariable tests.
|
|
||
| var clientMetric = clientMetricSentValues.First(); | ||
| Assert.AreEqual(m_Server.LocalClientId, clientMetric.Connection.Id); | ||
| Assert.AreEqual("MyServerRpc", clientMetric.Name); |
Contributor
There was a problem hiding this comment.
Nit, would recommend using nameof(RpcTestComponent.MyServerRpc)
josiemessa
reviewed
Jul 12, 2021
| { | ||
| if (!m_NetworkGameObjects.ContainsKey(networkObjectId)) | ||
| { | ||
| m_NetworkGameObjects[networkObjectId] = new NetworkObjectIdentifier($"NetworkGameObject_{networkObjectId}", networkObjectId); |
There was a problem hiding this comment.
This looks slightly different to the other metrics, here we're updating two metrics, one is tracking network objects (which if this is something we should do I'll need to make changes to the network object spawned/destroyed metrics as these are just in their own metric type), and one is tracking the RPCs. In the other "Track" functions we're only updating one metric type.
| { | ||
| if (!MultiInstanceHelpers.Create(1, out m_Server, out NetworkManager[] clients)) | ||
| { | ||
| Debug.LogError("Failed to create instances"); |
There was a problem hiding this comment.
Assert will log the failure I think so we shouldn't need double logs?
Author
There was a problem hiding this comment.
Good to know, I will look at that and remove duplicates
| if (!MultiInstanceHelpers.Start(true, m_Server, clients)) | ||
| { | ||
| Debug.LogError("Failed to start instances"); | ||
| Assert.Fail("Failed to start instances"); |
becksebenius-unity
approved these changes
Jul 13, 2021
becksebenius-unity
added a commit
that referenced
this pull request
Aug 12, 2021
* Initial reference and NetworkMetrics * Implement profiling decorator and network metrics for named messages * Add dummy dispatch frame * Move dispatch frame * Move references * Fix namespace reference * Client Connection Ids and Game Object registration (#863) * Client Connection Ids and Game Object registration * Fixing connection to use local and avoid allocation at every new metrics * Fixed client id connections to point to other clients * Using IReadOnlyCollection instead of explicit List * Changing variable name to sender and receiver * feat: Add test for named message sent and named message received metrics (#861) * Add test for named message sent * Add test for named messages sent to multiple clients * Add test for named message received * Revert editor changes * Code review fixes * Merge test fixes * feat: Add implementation and tests for unnamed message metrics (#866) * Implement metrics for network variable deltas (#884) * Removing duplicate * Fix merge * Merge fix * Rename and move messaging metrics tests * Fix merge errors * Add helper to wait for metric values in tests (#896) * Add helper to wait for metric values in tests * Use helper in other tests classes * Fail when metric values haven't been found * Add conditional define for the tools library (#908) * Add conditional define for the tools library * Remove conditional define from test project * feat: Report metrics when network objects are spawned or destroyed/despawned. (#930) Create new object spawned/destroyed sent and received metrics. Co-authored-by: Benoit Doyon <[email protected]> * chore: Compile out file when tool isn’t present (#953) https://github.com/Unity-Technologies/com.unity.multiplayer.mlapi * feat: Add metrics for multiple objects spawned and destroyed (i.e. NetworkShow/Hide) (#935) Make use of existing network object spawn destroy metrics individually for each network object, so that sent metrics mirror the received metrics, and to avoid allocations. Co-authored-by: Benoit Doyon <[email protected]> * feat: RPC Event Net stat reporting (#954) * RPC Event implementation with test * Update based on comments * Fixed RPC byte size * Fix tests to use properly registered network prefabs (#962) * Updated to match tools PR that decouples network profiler references (#961) * Compilation fix * Feat: Server logs metrics (#956) * Experimental/netstats/review fixes (#970) * Track ownership change events (#931) * Move common metrics test initialization to utility class (#973) * Assert byte counts for metrics (#975) * Merge fixes * Fix buffer size tracking * Fix named message stream copyto * Make network object tests use the base class * test: added an empty test project that includes the tools package (#1016) * Added a test project that references the tools package * Reverted project.metafile change to try to diagnose a yamato failure * trying a different format for the project name * updated yamato files to avoid duplicate keys * fixed triggers and 'run all' jobs not using new name or respecting validate flag * re-added "test_" prefix to avoid too many changes to the structure in yamato * fixed incorrect pack dependency for non-first projects * Added utp to the integration project * fixed package tests always using the first project * Added a dummy asset to make sure that the test project assets folder is actually created * Removed "packages" list from tools test project to simplify list * fixed WaitForEndOfFrame issue * Compiled out metrics tests when tools package isn't present * Made sure all projects are tested from pull request even if they contain no packages * changed test_editors to be project-specific * fix mistake in previous * Updated tools package testing to 0.0.1-preview.1 * missed updating the packages-lock.json in previous commit * Code review fixes * Code review fixes * Code review fixes * Code review fixes * Force CI * Revert "Force CI" This reverts commit f0f4479. * fix: updated some missed cases where test_editors was still used (#1039) * Removed the incorrect "on" in the pull request trigger name * Fixed another place where I missed adding `project.` * Fixed ObjectDestroy metric not being sent when NetworkHide is used (#1041) * Avoid string allocation when printing variable name by sanitizing on variable initialization (#1043) * fix: pull request trigger name does not match the one configured in github branch protections (#1045) * fix: pull request trigger name does not match the one configured in github branch protections * Moved the old name to a "legacy" job so that the branch protected can be easily updated after merge * fix: ported over the actual trigger data for the legacy trigger so it runs automatically (#1047) * Fixing testproject-tools-integration failure Co-authored-by: Benoit Doyon <[email protected]> Co-authored-by: Jean-Sébastien Fauteux <[email protected]> Co-authored-by: Benoit Doyon <[email protected]> Co-authored-by: josiemessa <[email protected]> Co-authored-by: kvassall-unity <[email protected]>
mollstam
pushed a commit
to Keepsake-Games/com.unity.netcode.gameobjects
that referenced
this pull request
Feb 13, 2023
…ies#960) * Initial reference and NetworkMetrics * Implement profiling decorator and network metrics for named messages * Add dummy dispatch frame * Move dispatch frame * Move references * Fix namespace reference * Client Connection Ids and Game Object registration (Unity-Technologies#863) * Client Connection Ids and Game Object registration * Fixing connection to use local and avoid allocation at every new metrics * Fixed client id connections to point to other clients * Using IReadOnlyCollection instead of explicit List * Changing variable name to sender and receiver * feat: Add test for named message sent and named message received metrics (Unity-Technologies#861) * Add test for named message sent * Add test for named messages sent to multiple clients * Add test for named message received * Revert editor changes * Code review fixes * Merge test fixes * feat: Add implementation and tests for unnamed message metrics (Unity-Technologies#866) * Implement metrics for network variable deltas (Unity-Technologies#884) * Removing duplicate * Fix merge * Merge fix * Rename and move messaging metrics tests * Fix merge errors * Add helper to wait for metric values in tests (Unity-Technologies#896) * Add helper to wait for metric values in tests * Use helper in other tests classes * Fail when metric values haven't been found * Add conditional define for the tools library (Unity-Technologies#908) * Add conditional define for the tools library * Remove conditional define from test project * feat: Report metrics when network objects are spawned or destroyed/despawned. (Unity-Technologies#930) Create new object spawned/destroyed sent and received metrics. Co-authored-by: Benoit Doyon <[email protected]> * chore: Compile out file when tool isn’t present (Unity-Technologies#953) https://github.com/Unity-Technologies/com.unity.multiplayer.mlapi * feat: Add metrics for multiple objects spawned and destroyed (i.e. NetworkShow/Hide) (Unity-Technologies#935) Make use of existing network object spawn destroy metrics individually for each network object, so that sent metrics mirror the received metrics, and to avoid allocations. Co-authored-by: Benoit Doyon <[email protected]> * feat: RPC Event Net stat reporting (Unity-Technologies#954) * RPC Event implementation with test * Update based on comments * Fixed RPC byte size * Fix tests to use properly registered network prefabs (Unity-Technologies#962) * Updated to match tools PR that decouples network profiler references (Unity-Technologies#961) * Compilation fix * Feat: Server logs metrics (Unity-Technologies#956) * Experimental/netstats/review fixes (Unity-Technologies#970) * Track ownership change events (Unity-Technologies#931) * Move common metrics test initialization to utility class (Unity-Technologies#973) * Assert byte counts for metrics (Unity-Technologies#975) * Merge fixes * Fix buffer size tracking * Fix named message stream copyto * Make network object tests use the base class * test: added an empty test project that includes the tools package (Unity-Technologies#1016) * Added a test project that references the tools package * Reverted project.metafile change to try to diagnose a yamato failure * trying a different format for the project name * updated yamato files to avoid duplicate keys * fixed triggers and 'run all' jobs not using new name or respecting validate flag * re-added "test_" prefix to avoid too many changes to the structure in yamato * fixed incorrect pack dependency for non-first projects * Added utp to the integration project * fixed package tests always using the first project * Added a dummy asset to make sure that the test project assets folder is actually created * Removed "packages" list from tools test project to simplify list * fixed WaitForEndOfFrame issue * Compiled out metrics tests when tools package isn't present * Made sure all projects are tested from pull request even if they contain no packages * changed test_editors to be project-specific * fix mistake in previous * Updated tools package testing to 0.0.1-preview.1 * missed updating the packages-lock.json in previous commit * Code review fixes * Code review fixes * Code review fixes * Code review fixes * Force CI * Revert "Force CI" This reverts commit f0f4479. * fix: updated some missed cases where test_editors was still used (Unity-Technologies#1039) * Removed the incorrect "on" in the pull request trigger name * Fixed another place where I missed adding `project.` * Fixed ObjectDestroy metric not being sent when NetworkHide is used (Unity-Technologies#1041) * Avoid string allocation when printing variable name by sanitizing on variable initialization (Unity-Technologies#1043) * fix: pull request trigger name does not match the one configured in github branch protections (Unity-Technologies#1045) * fix: pull request trigger name does not match the one configured in github branch protections * Moved the old name to a "legacy" job so that the branch protected can be easily updated after merge * fix: ported over the actual trigger data for the legacy trigger so it runs automatically (Unity-Technologies#1047) * Fixing testproject-tools-integration failure Co-authored-by: Benoit Doyon <[email protected]> Co-authored-by: Jean-Sébastien Fauteux <[email protected]> Co-authored-by: Benoit Doyon <[email protected]> Co-authored-by: josiemessa <[email protected]> Co-authored-by: kvassall-unity <[email protected]>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
No description provided.