Skip to content

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

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.

Sign up for GitHub

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
merged 6 commits into from
Sep 16, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
46a8a76
fix: Report messages properly that are locally executed as 0 bytes
kvassall-unity Sep 13, 2021
e20f7b4
Fix: we can run tests from internal classes
kvassall-unity Sep 15, 2021
fa83415
fix: whitespace
kvassall-unity Sep 15, 2021
190d122
Merge branch 'develop' into feature/track_local_execution
kvassall-unity Sep 16, 2021
04da50a
chore: Fixing namespace
kvassall-unity Sep 16, 2021
e73c020
Merge branch 'develop' into feature/track_local_execution
kvassall-unity Sep 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,12 +222,18 @@ internal void __endSendClientRpc(NetworkSerializer serializer, uint rpcMethodId,
#if DEVELOPMENT_BUILD || UNITY_EDITOR
if (NetworkManager.__rpc_name_table.TryGetValue(rpcMethodId, out var rpcMethodName))
{
NetworkManager.NetworkMetrics.TrackRpcSent(
NetworkManager.ConnectedClients.Select(x => x.Key).ToArray(),
NetworkObjectId,
rpcMethodName,
__getTypeName(),
messageSize);
foreach (var client in NetworkManager.ConnectedClients)
{
var bytesReported = NetworkManager.LocalClientId == client.Key
? 0
: messageSize;
NetworkManager.NetworkMetrics.TrackRpcSent(
client.Key,
NetworkObjectId,
rpcMethodName,
__getTypeName(),
bytesReported);
Comment on lines +227 to +235
Copy link
Contributor

@ian-m-unity ian-m-unity Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
#endif
}
Expand Down Expand Up @@ -582,13 +588,17 @@ private void NetworkVariableUpdate(ulong clientId, int behaviourIndex)
m_NetworkVariableIndexesToReset.Add(k);
}

var bytesReported = NetworkManager.LocalClientId == clientId
? 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

: bufferSizeCapture.Flush();

NetworkManager.NetworkMetrics.TrackNetworkVariableDeltaSent(
clientId,
NetworkObjectId,
name,
NetworkVariableFields[k].Name,
__getTypeName(),
bufferSizeCapture.Flush());
bytesReported);
}
}

Expand Down Expand Up @@ -678,13 +688,17 @@ internal void HandleNetworkVariableDeltas(Stream stream, ulong clientId)
long readStartPos = stream.Position;

NetworkVariableFields[i].ReadDelta(stream, NetworkManager.IsServer);

var bytesReported = NetworkManager.LocalClientId == clientId
? 0
: stream.Length;
NetworkManager.NetworkMetrics.TrackNetworkVariableDeltaReceived(
clientId,
NetworkObjectId,
name,
NetworkVariableFields[i].Name,
__getTypeName(),
stream.Length);
bytesReported);

(stream as NetworkBuffer).SkipPadBits();

Expand Down
10 changes: 8 additions & 2 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,7 +1199,10 @@ internal void HandleIncomingData(ulong clientId, ArraySegment<byte> payload, flo
{
var messageType = (MessageQueueContainer.MessageType)messageStream.ReadByte();
MessageHandler.MessageReceiveQueueItem(clientId, messageStream, receiveTime, messageType);
NetworkMetrics.TrackNetworkMessageReceived(clientId, MessageQueueContainer.GetMessageTypeName(messageType), payload.Count);
var bytesReported = LocalClientId == clientId
? 0
: payload.Count;
NetworkMetrics.TrackNetworkMessageReceived(clientId, MessageQueueContainer.GetMessageTypeName(messageType), bytesReported);
}
#if DEVELOPMENT_BUILD || UNITY_EDITOR
s_HandleIncomingData.End();
Expand All @@ -1209,7 +1212,10 @@ internal void HandleIncomingData(ulong clientId, ArraySegment<byte> payload, flo
private void ReceiveCallback(NetworkBuffer messageBuffer, MessageQueueContainer.MessageType messageType, ulong clientId, float receiveTime)
{
MessageHandler.MessageReceiveQueueItem(clientId, messageBuffer, receiveTime, messageType);
NetworkMetrics.TrackNetworkMessageReceived(clientId, MessageQueueContainer.GetMessageTypeName(messageType), messageBuffer.Length);
var bytesReported = LocalClientId == clientId
? 0
: messageBuffer.Length;
NetworkMetrics.TrackNetworkMessageReceived(clientId, MessageQueueContainer.GetMessageTypeName(messageType), bytesReported);
}

/// <summary>
Expand Down
5 changes: 4 additions & 1 deletion com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,10 @@ public void NetworkHide(ulong clientId)
nonNullContext.NetworkWriter.WriteUInt64Packed(NetworkObjectId);

var size = bufferSizeCapture.StopMeasureSegment();
NetworkManager.NetworkMetrics.TrackObjectDestroySent(clientId, NetworkObjectId, name, size);
var bytesReported = NetworkManager.LocalClientId == clientId
? 0
: size;
NetworkManager.NetworkMetrics.TrackObjectDestroySent(clientId, NetworkObjectId, name, bytesReported);
}
}
}
Expand Down
15 changes: 12 additions & 3 deletions com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ internal CustomMessagingManager(NetworkManager networkManager)
internal void InvokeUnnamedMessage(ulong clientId, Stream stream)
{
OnUnnamedMessage?.Invoke(clientId, stream);
m_NetworkManager.NetworkMetrics.TrackUnnamedMessageReceived(clientId, stream.SafeGetLengthOrDefault());
var bytesReported = m_NetworkManager.LocalClientId == clientId
? 0
: stream.SafeGetLengthOrDefault();
m_NetworkManager.NetworkMetrics.TrackUnnamedMessageReceived(clientId, bytesReported);
}

/// <summary>
Expand Down Expand Up @@ -71,7 +74,10 @@ public void SendUnnamedMessage(ulong clientId, NetworkBuffer messageBuffer, Netw
if (context != null)
{
using var nonNullContext = (InternalCommandContext)context;
m_NetworkManager.NetworkMetrics.TrackUnnamedMessageSent(clientId, messageBuffer.Position);
var bytesReported = m_NetworkManager.LocalClientId == clientId
? 0
: messageBuffer.Position;
m_NetworkManager.NetworkMetrics.TrackUnnamedMessageSent(clientId, bytesReported);
messageBuffer.Position = 0;
messageBuffer.CopyTo(nonNullContext.NetworkWriter.GetStream());
}
Expand Down Expand Up @@ -197,7 +203,10 @@ public void SendNamedMessage(string messageName, ulong clientId, Stream messageS

var size = bufferSizeCapture.StopMeasureSegment();

m_NetworkManager.NetworkMetrics.TrackNamedMessageSent(clientId, messageName, size);
var bytesReported = m_NetworkManager.LocalClientId == clientId
? 0
: size;
m_NetworkManager.NetworkMetrics.TrackNamedMessageSent(clientId, messageName, bytesReported);
}
}

Expand Down
20 changes: 16 additions & 4 deletions com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,10 @@ public void HandleAddObject(ulong clientId, Stream stream)
var networkObject = NetworkManager.SpawnManager.CreateLocalNetworkObject(softSync, prefabHash, ownerClientId, parentNetworkId, pos, rot, isReparented);
networkObject.SetNetworkParenting(isReparented, latestParent);
NetworkManager.SpawnManager.SpawnNetworkObjectLocally(networkObject, networkId, softSync, isPlayerObject, ownerClientId, stream, true, false);
m_NetworkManager.NetworkMetrics.TrackObjectSpawnReceived(clientId, networkObject.NetworkObjectId, networkObject.name, stream.Length);
var bytesReported = NetworkManager.LocalClientId == clientId
? 0
: stream.Length;
m_NetworkManager.NetworkMetrics.TrackObjectSpawnReceived(clientId, networkObject.NetworkObjectId, networkObject.name, bytesReported);
}

public void HandleDestroyObject(ulong clientId, Stream stream)
Expand All @@ -136,7 +139,10 @@ public void HandleDestroyObject(ulong clientId, Stream stream)
return;
}

m_NetworkManager.NetworkMetrics.TrackObjectDestroyReceived(clientId, networkObjectId, networkObject.name, stream.Length);
var bytesReported = NetworkManager.LocalClientId == clientId
? 0
: stream.Length;
m_NetworkManager.NetworkMetrics.TrackObjectDestroyReceived(clientId, networkObjectId, networkObject.name, bytesReported);
NetworkManager.SpawnManager.OnDespawnObject(networkObject, true);
}

Expand Down Expand Up @@ -180,7 +186,10 @@ public void HandleChangeOwner(ulong clientId, Stream stream)
networkObject.InvokeBehaviourOnGainedOwnership();
}

NetworkManager.NetworkMetrics.TrackOwnershipChangeReceived(clientId, networkObject.NetworkObjectId, networkObject.name, stream.Length);
var bytesReported = NetworkManager.LocalClientId == clientId
? 0
: stream.Length;
NetworkManager.NetworkMetrics.TrackOwnershipChangeReceived(clientId, networkObject.NetworkObjectId, networkObject.name, bytesReported);
}

public void HandleTimeSync(ulong clientId, Stream stream)
Expand Down Expand Up @@ -279,7 +288,10 @@ public void HandleNetworkLog(ulong clientId, Stream stream)
using var reader = PooledNetworkReader.Get(stream);
var length = stream.Length;
var logType = (NetworkLog.LogType)reader.ReadByte();
m_NetworkManager.NetworkMetrics.TrackServerLogReceived(clientId, (uint)logType, length);
var bytesReported = NetworkManager.LocalClientId == clientId
? 0
: length;
m_NetworkManager.NetworkMetrics.TrackServerLogReceived(clientId, (uint)logType, bytesReported);
string message = reader.ReadStringPacked();

switch (logType)
Expand Down
16 changes: 13 additions & 3 deletions com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1116,7 +1116,10 @@ private void OnServerLoadedScene(Scene scene)
SceneEventData.OnWrite(nonNullContext.NetworkWriter);

var size = bufferSizeCapture.StopMeasureSegment();
m_NetworkManager.NetworkMetrics.TrackSceneEventSent(clientId, (uint)SceneEventData.SceneEventType, scene.name, size);
var bytesReported = m_NetworkManager.LocalClientId == clientId
? 0
: size;
m_NetworkManager.NetworkMetrics.TrackSceneEventSent(clientId, (uint)SceneEventData.SceneEventType, scene.name, bytesReported);
}
else
{
Expand Down Expand Up @@ -1230,8 +1233,11 @@ internal void SynchronizeNetworkObjects(ulong clientId)
ClientSynchEventData.OnWrite(nonNullContext.NetworkWriter);

var size = bufferSizeCapture.StopMeasureSegment();
var bytesReported = m_NetworkManager.LocalClientId == clientId
? 0
: size;
Comment on lines +1236 to +1238
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ian-m-unity ian-m-unity Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

m_NetworkManager.NetworkMetrics.TrackSceneEventSent(
clientId, (uint)ClientSynchEventData.SceneEventType, "", size);
clientId, (uint)ClientSynchEventData.SceneEventType, "", bytesReported);
}

// Notify the local server that the client has been sent the SceneEventData.SceneEventTypes.S2C_Event_Sync event
Expand Down Expand Up @@ -1577,8 +1583,12 @@ internal void HandleSceneEvent(ulong clientId, Stream stream)
SceneEventData.OnRead(reader);
NetworkReaderPool.PutBackInPool(reader);

var bytesReported = m_NetworkManager.LocalClientId == clientId
? 0
: stream.Length;

m_NetworkManager.NetworkMetrics.TrackSceneEventReceived(
clientId, (uint)SceneEventData.SceneEventType, ScenesInBuild[(int)SceneEventData.SceneIndex], stream.Length);
clientId, (uint)SceneEventData.SceneEventType, ScenesInBuild[(int)SceneEventData.SceneIndex], bytesReported);

if (SceneEventData.IsSceneEventClientSide())
{
Expand Down
24 changes: 20 additions & 4 deletions com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,10 @@ internal void RemoveOwnership(NetworkObject networkObject)

foreach (var client in NetworkManager.ConnectedClients)
{
NetworkManager.NetworkMetrics.TrackOwnershipChangeSent(client.Key, networkObject.NetworkObjectId, networkObject.name, size);
var bytesReported = NetworkManager.LocalClientId == client.Key
? 0
: size;
NetworkManager.NetworkMetrics.TrackOwnershipChangeSent(client.Key, networkObject.NetworkObjectId, networkObject.name, bytesReported);
}
}
}
Expand Down Expand Up @@ -163,7 +166,10 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)
var size = bufferSizeCapture.StopMeasureSegment();
foreach (var client in NetworkManager.ConnectedClients)
{
NetworkManager.NetworkMetrics.TrackOwnershipChangeSent(client.Key, networkObject.NetworkObjectId, networkObject.name, size);
var bytesReported = NetworkManager.LocalClientId == client.Key
? 0
: size;
NetworkManager.NetworkMetrics.TrackOwnershipChangeSent(client.Key, networkObject.NetworkObjectId, networkObject.name, bytesReported);
}
}
}
Expand Down Expand Up @@ -328,6 +334,7 @@ internal void SpawnNetworkObjectLocally(NetworkObject networkObject, ulong netwo
SpawnedObjectsList.Add(networkObject);

NetworkManager.NetworkMetrics.TrackNetworkObject(networkObject);
NetworkManager.NetworkMetrics.TrackObjectSpawnSent(NetworkManager.LocalClientId, networkObject.NetworkObjectId, networkObject.name, 0);

if (ownerClientId != null)
{
Expand Down Expand Up @@ -389,7 +396,10 @@ internal void SendSpawnCallForObject(ulong clientId, NetworkObject networkObject
WriteSpawnCallForObject(nonNullContext.NetworkWriter, clientId, networkObject);

var size = bufferSizeCapture.StopMeasureSegment();
NetworkManager.NetworkMetrics.TrackObjectSpawnSent(clientId, networkObject.NetworkObjectId, networkObject.name, size);
var bytesReported = NetworkManager.LocalClientId == clientId
? 0
: size;
NetworkManager.NetworkMetrics.TrackObjectSpawnSent(clientId, networkObject.NetworkObjectId, networkObject.name, bytesReported);
}

networkObject.MarkVariablesDirty();
Expand Down Expand Up @@ -669,7 +679,13 @@ internal void OnDespawnObject(NetworkObject networkObject, bool destroyGameObjec
nonNullContext.NetworkWriter.WriteUInt64Packed(networkObject.NetworkObjectId);

var size = bufferSizeCapture.StopMeasureSegment();
NetworkManager.NetworkMetrics.TrackObjectDestroySent(m_TargetClientIds, networkObject.NetworkObjectId, networkObject.name, size);
foreach (var targetClientId in m_TargetClientIds)
{
var bytesReported = NetworkManager.LocalClientId == targetClientId
? 0
: size;
NetworkManager.NetworkMetrics.TrackObjectDestroySent(targetClientId, networkObject.NetworkObjectId, networkObject.name, bytesReported);
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion com.unity.netcode.gameobjects/Tests/Runtime/Metrics/MessagingMetricsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
using System.Text;
using NUnit.Framework;
using Unity.Multiplayer.Tools.MetricTypes;
using Unity.Netcode.RuntimeTests.Metrics.Utlity;
using Unity.Netcode.RuntimeTests.Metrics.Utility;
using UnityEngine;
using UnityEngine.TestTools;

Expand Down
22 changes: 10 additions & 12 deletions com.unity.netcode.gameobjects/Tests/Runtime/Metrics/NetworkObjectMetricsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
using System.Linq;
using NUnit.Framework;
using Unity.Multiplayer.Tools.MetricTypes;
using Unity.Netcode.RuntimeTests.Metrics.Utlity;
using Unity.Netcode.RuntimeTests.Metrics.Utility;
using UnityEngine;
using UnityEngine.TestTools;

namespace Unity.Netcode.RuntimeTests.Metrics
{
public class NetworkObjectMetricsTests : SingleClientMetricTestBase
internal class NetworkObjectMetricsTests : SingleClientMetricTestBase
{
private const string k_NewNetworkObjectName = "TestNetworkObjectToSpawn";
private NetworkObject m_NewNetworkPrefab;
Expand Down Expand Up @@ -50,13 +50,12 @@ public IEnumerator TrackNetworkObjectSpawnSentMetric()

yield return waitForMetricEvent.WaitForMetricsReceived();

var objectSpawnedSentMetricValues = waitForMetricEvent.AssertMetricValuesHaveBeenFound();
Assert.AreEqual(1, objectSpawnedSentMetricValues.Count);
var metricValues = waitForMetricEvent.AssertMetricValuesHaveBeenFound();
Assert.AreEqual(2, metricValues.Count);

var objectSpawned = objectSpawnedSentMetricValues.First();
Assert.AreEqual(Client.LocalClientId, objectSpawned.Connection.Id);
var objectSpawned = metricValues.First();
Assert.AreEqual($"{k_NewNetworkObjectName}(Clone)", objectSpawned.NetworkId.Name);
Assert.AreNotEqual(0, objectSpawned.BytesCount);
AssertLocalAndRemoteMetricsSent(metricValues);
}

[UnityTest]
Expand Down Expand Up @@ -91,13 +90,12 @@ public IEnumerator TrackNetworkObjectDestroySentMetric()

yield return waitForMetricEvent.WaitForMetricsReceived();

var objectDestroyedSentMetricValues = waitForMetricEvent.AssertMetricValuesHaveBeenFound();
Assert.AreEqual(2, objectDestroyedSentMetricValues.Count); // As there's a client and server, this event is emitted twice.
var metricValues = waitForMetricEvent.AssertMetricValuesHaveBeenFound();
Assert.AreEqual(2, metricValues.Count); // As there's a client and server, this event is emitted twice.

var objectDestroyed = objectDestroyedSentMetricValues.Last();
Assert.AreEqual(Client.LocalClientId, objectDestroyed.Connection.Id);
var objectDestroyed = metricValues.Last();
Assert.AreEqual($"{k_NewNetworkObjectName}(Clone)", objectDestroyed.NetworkId.Name);
Assert.AreNotEqual(0, objectDestroyed.BytesCount);
AssertLocalAndRemoteMetricsSent(metricValues);
}

[UnityTest]
Expand Down
7 changes: 3 additions & 4 deletions com.unity.netcode.gameobjects/Tests/Runtime/Metrics/NetworkVariableMetricsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
using System.Linq;
using NUnit.Framework;
using Unity.Multiplayer.Tools.MetricTypes;
using Unity.Netcode.RuntimeTests.Metrics.Utlity;
using Unity.Netcode.RuntimeTests.Metrics.Utility;
using UnityEngine;
using UnityEngine.TestTools;

namespace Unity.Netcode.RuntimeTests.Metrics
{
public class NetworkVariableMetricsTests : SingleClientMetricTestBase
internal class NetworkVariableMetricsTests : SingleClientMetricTestBase
{
protected override Action<GameObject> UpdatePlayerPrefab => prefab => prefab.AddComponent<NetworkVariableComponent>();

Expand All @@ -25,8 +25,7 @@ public IEnumerator TrackNetworkVariableDeltaSentMetric()

var networkVariableDeltaSent = metricValues.First();
Assert.AreEqual(nameof(NetworkVariableComponent.MyNetworkVariable), networkVariableDeltaSent.Name);
Assert.AreEqual(Server.LocalClientId, networkVariableDeltaSent.Connection.Id);
Assert.AreNotEqual(0, networkVariableDeltaSent.BytesCount);
AssertLocalAndRemoteMetricsSent(metricValues);
}

[UnityTest]
Expand Down
Loading