fix: Report messages properly that are locally executed as 0 bytes #1162

kvassall-unity · 2021-09-09T19:08:10Z

No description provided.

becksebenius-unity · 2021-09-09T20:16:49Z

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

+                    var bytesReported = m_NetworkManager.LocalClientId == clientId
+                        ? 0
+                        : stream.Length;
+
                    if (SceneEventData.SceneEventType == SceneEventData.SceneEventTypes.S2C_Sync)
                    {
                        // For this event the server may be sending scene event data about multiple scenes, so we need
                        // to track a metric for each one.
                        foreach (var sceneIndex in SceneEventData.ScenesToSynchronize)
                        {
                            m_NetworkManager.NetworkMetrics.TrackSceneEventReceived(
-                               clientId, (uint)SceneEventData.SceneEventType, ScenesInBuild[(int)sceneIndex], stream.Length);
+                               clientId, (uint)SceneEventData.SceneEventType, ScenesInBuild[(int)sceneIndex], bytesReported);
                        }
                    }
                    else
                    {
                        // For all other scene event types, we are only dealing with one scene at a time, so we can read it
                        // from the SceneEventData directly.
                        m_NetworkManager.NetworkMetrics.TrackSceneEventReceived(
-                           clientId, (uint)SceneEventData.SceneEventType, ScenesInBuild[(int)SceneEventData.SceneIndex], stream.Length);
+                           clientId, (uint)SceneEventData.SceneEventType, ScenesInBuild[(int)SceneEventData.SceneIndex], bytesReported);


I think this is getting refactored in Josie's PR FYI

becksebenius-unity · 2021-09-09T20:19:00Z

Tests are failing with latest commit. Although I'm a little concerned that it's just the NetworkVariableDelta tests that are failing, not the others. These tests were supposed to validate byte counts.

0xFA11 · 2021-09-14T15:06:43Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

                            }

+                            var bytesReported = NetworkManager.LocalClientId == clientId
+                                ? 0


nit: a comment explaining why we return 0 here might be useful (like "not a failure but success with 0") — also applies to other 0 returning ternaries below.

0xFA11

🚀

becksebenius-unity · 2021-09-15T16:30:44Z

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

+                var bytesReported = m_NetworkManager.LocalClientId == clientId
+                    ? 0
+                    : size;


Late feedback on this part, but I wonder if it makes sense to add an internal utility function to network manager? Something like m_NetworkManager.GetBytesForMetricReporting(clientId, size);.

I mean at that point it could be part of the BufferSizeCapture utility class

Maybe, I don't know if BufferSizeCapture should know anything about client ids though. this eval only work if you know networkManager.LocalClientId

This is one statement. I don't see too huge of a reason to refactor a single statement into a function

I mainly proposed this because you're using the same pattern a bunch of times across the codebase, so it is a bit of a code smell. I agree that for such a small expression it's not a big deal

Yeah, I agree with Beck. I know it's just one statement, but anywhere these functions are used without this statement will be a bug that's inconsistent with the others, so I think there should be a function that can be called that takes care of this ternary as well

ian-m-unity · 2021-09-16T01:11:42Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

+                    var bytesReported = NetworkManager.LocalClientId == client.Key
+                        ? 0
+                        : messageSize;
+                    NetworkManager.NetworkMetrics.TrackRpcSent(
+                        client.Key,
+                        NetworkObjectId,
+                        rpcMethodName,
+                        __getTypeName(),
+                        bytesReported);


This pattern of computing bytesReported is repeated so often that personally I think it should be extracted out to other function(s) before merging. As is this is a little fragile, because any of these Track functions called without the ternary will be result in bugs as their behavior will be inconsistent with the rest. I think this work could be done in the NetworkMetrics Track functions themselves, so that at least all of this is taken care of in one file if it can't be done in a single function.

becksebenius-unity · 2021-09-16T15:36:42Z

com.unity.netcode.gameobjects/Tests/Runtime/Metrics/RpcMetricsTests.cs

 using NUnit.Framework;
 using Unity.Multiplayer.Tools.MetricTypes;
+using Unity.Netcode.RuntimeTests.Metrics.Utility;
 using Unity.Netcode.RuntimeTests.Metrics.Utlity;


Shouldn't this be removed now too?

becksebenius-unity · 2021-09-16T15:39:20Z

I think there's still a few missing test updates - it looks like the test pattern worked for the ones you added which is great, but you've modified more cases than the tests you've updated and I think it's important that the change in behavior is verified for all the cases. For example, the Object Destroy case was updated to report 0 for local execution but the test wasn't updated to verify it. I think it should be as simple as applying your new method to those tests though

kvassall-unity · 2021-09-16T16:33:03Z

I think there's still a few missing test updates - it looks like the test pattern worked for the ones you added which is great, but you've modified more cases than the tests you've updated and I think it's important that the change in behavior is verified for all the cases. For example, the Object Destroy case was updated to report 0 for local execution but the test wasn't updated to verify it. I think it should be as simple as applying your new method to those tests though

TrackNetworkObjectDestroySentMetric
This already has the appropriate Assert call.
Could you just mark the tests that you think have missing tests?
I thought i got them all

becksebenius-unity · 2021-09-16T18:41:52Z

I think there's still a few missing test updates - it looks like the test pattern worked for the ones you added which is great, but you've modified more cases than the tests you've updated and I think it's important that the change in behavior is verified for all the cases. For example, the Object Destroy case was updated to report 0 for local execution but the test wasn't updated to verify it. I think it should be as simple as applying your new method to those tests though

TrackNetworkObjectDestroySentMetric
This already has the appropriate Assert call.
Could you just mark the tests that you think have missing tests?
I thought i got them all

Oh, sorry about that. Somehow I missed it!

…ty-Technologies/com.unity.multiplayer.mlapi into sam/feature/client-network-transform * 'sam/feature/client-network-transform' of github.com:Unity-Technologies/com.unity.multiplayer.mlapi: fix: Report messages properly that are locally executed as 0 bytes (#1162)

…transform-teleport * sam/feature/client-network-transform: adding some tests todo all current tests pass fixed tests, interpolation was depending on network manager singleton, this didn't do well in multiinstance tests removing useless comments removing useless sample, adding delta input per Matt's design missing private adding compression, owner check, dirty check adding input based transform sample fixing CI issues test: build multiprocesstestplayer and add it to project tests job (#1174) reenabling net transform tests fix: Report messages properly that are locally executed as 0 bytes (#1162)

…nity-Technologies#1162) * fix: Report messages properly that are locally executed as 0 bytes * Fix: we can run tests from internal classes * fix: whitespace * chore: Fixing namespace

kvassall-unity requested review from a team September 9, 2021 19:08

becksebenius-unity reviewed Sep 9, 2021

View reviewed changes

kvassall-unity force-pushed the feature/track_local_execution branch from f711f88 to 021d8a0 Compare September 13, 2021 19:22

kvassall-unity closed this Sep 13, 2021

kvassall-unity force-pushed the feature/track_local_execution branch from 021d8a0 to a02dfee Compare September 13, 2021 19:25

kvassall-unity reopened this Sep 13, 2021

kvassall-unity requested a review from becksebenius-unity September 13, 2021 21:22

0xFA11 reviewed Sep 14, 2021

View reviewed changes

0xFA11 approved these changes Sep 14, 2021

View reviewed changes

kvassall-unity force-pushed the feature/track_local_execution branch from eea5634 to 5c5647c Compare September 14, 2021 21:22

becksebenius-unity reviewed Sep 15, 2021

View reviewed changes

fix: Report messages properly that are locally executed as 0 bytes

46a8a76

kvassall-unity force-pushed the feature/track_local_execution branch from 42e1133 to 46a8a76 Compare September 15, 2021 16:54

kvassall-unity added 2 commits September 15, 2021 15:04

Fix: we can run tests from internal classes

e20f7b4

fix: whitespace

fa83415

ian-m-unity reviewed Sep 16, 2021

View reviewed changes

Merge branch 'develop' into feature/track_local_execution

190d122

becksebenius-unity reviewed Sep 16, 2021

View reviewed changes

kvassall-unity and others added 2 commits September 16, 2021 13:24

chore: Fixing namespace

04da50a

Merge branch 'develop' into feature/track_local_execution

e73c020

becksebenius-unity approved these changes Sep 16, 2021

View reviewed changes

kvassall-unity merged commit 16d9850 into develop Sep 16, 2021

kvassall-unity deleted the feature/track_local_execution branch September 16, 2021 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Report messages properly that are locally executed as 0 bytes #1162

fix: Report messages properly that are locally executed as 0 bytes #1162

Uh oh!

kvassall-unity commented Sep 9, 2021

Uh oh!

becksebenius-unity Sep 9, 2021

Uh oh!

becksebenius-unity commented Sep 9, 2021

Uh oh!

0xFA11 Sep 14, 2021

Uh oh!

0xFA11 left a comment

Uh oh!

becksebenius-unity Sep 15, 2021

Uh oh!

bendoyon Sep 15, 2021

Uh oh!

becksebenius-unity Sep 15, 2021

Uh oh!

kvassall-unity Sep 15, 2021

Uh oh!

becksebenius-unity Sep 15, 2021

Uh oh!

ian-m-unity Sep 16, 2021 •

edited

Loading

Uh oh!

ian-m-unity Sep 16, 2021 •

edited

Loading

Uh oh!

becksebenius-unity Sep 16, 2021

Uh oh!

becksebenius-unity commented Sep 16, 2021 •

edited

Loading

Uh oh!

kvassall-unity commented Sep 16, 2021

Uh oh!

becksebenius-unity commented Sep 16, 2021

Uh oh!

Uh oh!

fix: Report messages properly that are locally executed as 0 bytes #1162

fix: Report messages properly that are locally executed as 0 bytes #1162

Uh oh!

Conversation

kvassall-unity commented Sep 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

becksebenius-unity commented Sep 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xFA11 left a comment

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!

ian-m-unity Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ian-m-unity Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

becksebenius-unity commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvassall-unity commented Sep 16, 2021

Uh oh!

becksebenius-unity commented Sep 16, 2021

Uh oh!

Uh oh!

ian-m-unity Sep 16, 2021 •

edited

Loading

ian-m-unity Sep 16, 2021 •

edited

Loading

becksebenius-unity commented Sep 16, 2021 •

edited

Loading