Client Connection Ids and Game Object registration #863

Rosme · 2021-05-27T19:41:11Z

No description provided.

bendoyon · 2021-05-27T19:44:39Z

com.unity.multiplayer.mlapi/Runtime/Messaging/InternalMessageHandler.cs

@@ -83,6 +83,7 @@ public void HandleConnectionApproved(ulong clientId, Stream stream, float receiv

                NetworkManager.ConnectedClients.Add(NetworkManager.LocalClientId, new NetworkClient { ClientId = NetworkManager.LocalClientId });

+                m_NetworkManager.NetworkMetrics.TrackConnection(NetworkManager.LocalClientId);


What's the advantage of this as opposed to fetching the connection ID in the network manager on every metrics call?

I just realized I forgot to reuse it elsewhere, but I was under the impression at the moment that we could reuse the data instead of reallocation. But let me change this in a moment.

becksebenius-unity · 2021-05-27T20:31:44Z

com.unity.multiplayer.mlapi/Runtime/Messaging/InternalMessageHandler.cs

@@ -408,7 +407,7 @@ public void HandleNamedMessage(ulong clientId, Stream stream)
        {
            PerformanceDataManager.Increment(ProfilerConstants.NamedMessageReceived);
            ProfilerStatManager.NamedMessage.Record();
-            
+


Please leave these types of changes out since it's just whitespace, I'm guessing you modified this and then undid the changes later but no need to touch the file with this

It was a mistake. I'll revert that.

becksebenius-unity · 2021-05-27T20:35:14Z

com.unity.multiplayer.mlapi/Runtime/Metrics/NetworkMetrics.cs

+        {
+            if (!m_NetworkGameObjects.ContainsKey(networkObject.NetworkObjectId))
+            {
+                m_NetworkGameObjects[networkObject.NetworkObjectId] = new NetworkObjectIdentifier(networkObject.name, networkObject.NetworkObjectId);


I'm not sure if NetworkObjectIdentifier needs to be cached like this. It is a struct so creating the instance is free. Also this means that changes to the network object's name will not appear in the profiler.

Although .name might allocate memory since it pulls from native - in which case cashing might still be preferred to avoid the allocation?

This sort of thing add complexity, and we can only guess at the benefits. I'm a fan of keeping this stuff as simple as possible, and then optimizing when we have cold hard data to back it up.

If we have to optimize part of this, the solution might turn out to be something else entirely.

Make it work, make it clean, make it fast. In that order.

Yes I was under that impression that caching would be beneficial if we're going to be reusing the structures a lot.

I think the cost isn't worth the incidental complexity it adds to the code.

becksebenius-unity · 2021-05-27T20:36:10Z

com.unity.multiplayer.mlapi/Runtime/Metrics/NetworkMetrics.cs

        }

        public void TrackNamedMessageReceived(string messageName, ulong bytesCount)
        {
-            m_NamedMessageReceivedEvent.Mark(new NamedMessageEvent(new ConnectionInfo(m_NetworkManager.LocalClientId), messageName, bytesCount));
+            m_NamedMessageReceivedEvent.Mark(new NamedMessageEvent(m_LocalConnectionInfo, messageName, bytesCount));


I'm confused at why this is m_LocalConnectionInfo. Shouldn't connection info be a parameter to this method? Aren't we tracking "I sent a named message to [connection]" or "I received a named message from [connection]"?

Same here, when I asked this question JS answered and it made sense for what we want to see in the profiler.

But maybe this is just a question of renaming variables and parameters to make it crystal clear what we're expecting.

No. It's the local connection that received a Named Message or sent it.

This doesn't make sense to me. Local connection is always involved in the transaction, and it seems pointless to mark that. What's beneficial to the users is to know which other connection is involved in the transaction. This code will cause all data to be grouped under a single connection id which doesn't provide any user value

What if you remote profile? The connection will be different than the local one. The current structure that we've agreed doesn't have both connection information. It's always been about the local connection.

Remote or not, you always implicitly know that the instance that you're profiling will be involved in the transaction. We don't need both connection information, I'm saying that the connection attached to our data should be the connection that we don't implicitly know about. Attaching the local connection information to the data provides no value to the user because it's information they already implicitly know

Here's an example. If you send a client RPC, it sends it to all clients. I would expect that there is an RPCSent event delivered to the profiler for each connection because in actuality the profiled instance just sent N RPCs, not one.

After an offline discussion with @bendoyon and @becksebenius-unity . We've aligned that indeed this should be the outgoing and incoming connections. A misunderstanding came from how we currently render the data in the profiler. Ultimately, we are profiling a connection "A" and it's either sending to a connection "B" or receiving it from it and this should reflect that.

becksebenius-unity · 2021-06-03T17:47:16Z

com.unity.multiplayer.mlapi/Runtime/Metrics/NetworkMetrics.cs

+            m_NamedMessageSentEvent.Mark(new NamedMessageEvent(new ConnectionInfo(clientId), messageName, bytesCount));
+        }
+
+        public void TrackNamedMessageSent(List<ulong> clientIds, string messageName, ulong bytesCount)


Nit, but could be good to use IReadOnlyList<ulong> so that it's explicit that this method doesn't produce side effects on the list

IReadOnlyCollection :)

becksebenius-unity · 2021-06-03T17:48:20Z

com.unity.multiplayer.mlapi/Runtime/Metrics/NetworkMetrics.cs

-        void TrackNamedMessageReceived(string messageName, ulong bytesCount);
+        void TrackNamedMessageSent(ulong clientId, string messageName, ulong bytesCount);
+
+        void TrackNamedMessageSent(List<ulong> clientIds, string messageName, ulong bytesCount);


Oh man, I wish we had c# 8 because this could just be a default interface implementation

bendoyon · 2021-06-03T18:21:45Z

com.unity.multiplayer.mlapi/Runtime/Metrics/NetworkMetrics.cs

+            }
+        }
+
+        public void TrackNamedMessageSent(ulong clientId, string messageName, ulong bytesCount)


I think we should be very explicit about client IDs and really call them "sender" and "receiver" whenever we use them.

I'll update this and merge.

…stats-dispatcher_MTT-730-connection-gameobjectid

* 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 <benoit.doyon@unity3d.com> * 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 <benoit.doyon@unity3d.com> * 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 <benoit.doyon@unity3d.com> Co-authored-by: Jean-Sébastien Fauteux <js.fauteux@unity3d.com> Co-authored-by: Benoit Doyon <46577133+bendoyon@users.noreply.github.com> Co-authored-by: josiemessa <josie@unity3d.com> Co-authored-by: kvassall-unity <68920108+kvassall-unity@users.noreply.github.com>

…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 <benoit.doyon@unity3d.com> * 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 <benoit.doyon@unity3d.com> * 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 <benoit.doyon@unity3d.com> Co-authored-by: Jean-Sébastien Fauteux <js.fauteux@unity3d.com> Co-authored-by: Benoit Doyon <46577133+bendoyon@users.noreply.github.com> Co-authored-by: josiemessa <josie@unity3d.com> Co-authored-by: kvassall-unity <68920108+kvassall-unity@users.noreply.github.com>

Client Connection Ids and Game Object registration

7673dec

Rosme requested a review from a team May 27, 2021 19:41

bendoyon reviewed May 27, 2021

View reviewed changes

Fixing connection to use local and avoid allocation at every new metrics

b71d6ab

becksebenius-unity reviewed May 27, 2021

View reviewed changes

Fixed client id connections to point to other clients

2752384

becksebenius-unity approved these changes Jun 3, 2021

View reviewed changes

Using IReadOnlyCollection instead of explicit List

3df98a0

bendoyon reviewed Jun 3, 2021

View reviewed changes

JS Fauteux added 2 commits June 3, 2021 14:23

Merge branch 'experimental/netstats-dispatcher' into experimental/net…

71051d0

…stats-dispatcher_MTT-730-connection-gameobjectid

Changing variable name to sender and receiver

8d30466

Rosme merged commit 103cd52 into experimental/netstats-dispatcher Jun 3, 2021

Rosme deleted the experimental/netstats-dispatcher_MTT-730-connection-gameobjectid branch June 3, 2021 18:30

		@@ -83,6 +83,7 @@ public void HandleConnectionApproved(ulong clientId, Stream stream, float receiv

		NetworkManager.ConnectedClients.Add(NetworkManager.LocalClientId, new NetworkClient { ClientId = NetworkManager.LocalClientId });

		m_NetworkManager.NetworkMetrics.TrackConnection(NetworkManager.LocalClientId);

Client Connection Ids and Game Object registration #863

Client Connection Ids and Game Object registration #863

Uh oh!

Conversation

Rosme commented May 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!