feat: Extract Metrics from Network Pipeline Stage #1583

Rosme · 2022-01-13T20:13:52Z

Move the data from pipeline stage to UnityTransport adapter and network metrics.

Testing and Documentation

No tests have been added.
No documentation changes or additions were necessary.

simon-lemay-unity · 2022-01-13T21:00:48Z

You probably want to reset the counters in the InitializeConnection method of the pipeline. Pipeline buffers either use uninitialized memory, or reuse the buffers from a previous connection. In both cases that could lead to erroneous metrics.

simon-lemay-unity · 2022-01-13T21:17:25Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

+
+
+                //TODO: Get the correct connection instead of m_ServerConnection
+                m_Driver.GetPipelineBuffers(m_LastSendPipeline, NetworkPipelineStageCollection.GetStageId(typeof(NetworkMetricsPipelineStage)),


What is the reason for doing this only for the last pipeline sent to? There could have been sends on multiple pipelines between two Update calls.

Also for reference, every pipeline gets its own buffers. In UnityTransport there are three pipelines defined (m_UnreliableFragmentedPipeline, m_UnreliableSequencedFragmentedPipeline, and m_ReliableSequencedFragmentedPipeline). They'll each have their own network metrics context. So basically every connection will get three different metrics contexts.

Oh, I was under the impression it was a single pipeline. That's good to know.

simon-lemay-unity · 2022-01-19T17:07:19Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

+        {
+            var networkConnection =  ParseClientId(transportClientId);
+
+            //Two pipelines, unreliable sequence and reliable fragmented


There are actually 3 pipelines now. I added another one for unreliable (non-sequenced) delivery. You'll also need the change from PR #1569 for it to work properly with the metrics pipeline stage. (I guess you could just apply the changes to this PR and I'll close the other one.)

I'll apply the change. It makes sense. Will fetch the last develop with the third pipeline.

Comment needs updated (or maybe just removed?)

I don't see that the change from #1569 has been applied to this branch. Without it, trying to access the metrics pipeline buffers from a development will fail.

Yeah, I would just remove the comment. Better to have this kind of thing in an enum than a comment.

becksebenius-unity · 2022-01-19T17:05:26Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

@@ -517,13 +517,67 @@ private void Update()
                {
                    ;
                }
+
+#if MULTIPLAYER_TOOLS
+                if (NetworkManager.Singleton.IsServer)


Might be worth breaking into a named function to avoid overloading Update

becksebenius-unity · 2022-01-19T17:06:13Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

+            {
+                var pipeline = i == 0 ? m_UnreliableSequencedPipeline : m_ReliableSequencedFragmentedPipeline;
+
+#if ENABLE_UNITY_COLLECTIONS_CHECKS


Is this an existing pattern? Definitely gets the job done, a little awkward though

becksebenius-unity · 2022-01-19T17:07:51Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

+            var networkConnection =  ParseClientId(transportClientId);
+
+            //Two pipelines, unreliable sequence and reliable fragmented
+            for (byte i = 0; i < 2; ++i)


Should 2 be a const instead of leaving the comment?

If there's only 2 pipelines, should this just be two explicit function calls and pass in the pipeline as a parameter? The for loop seems kind of unnecessarily complex control flow since we have a very small and const-size set of pipelines that aren't actually in a collection that we can iterate over.

becksebenius-unity · 2022-01-19T17:08:19Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

+#endif
+                {
+                    //Don't need to dispose of the buffers, they are filled with data pointers. 
+                    m_Driver.GetPipelineBuffers(pipeline, NetworkPipelineStageCollection.GetStageId(typeof(NetworkMetricsPipelineStage)),


nit, suggest breaking into 1 line per parameter for better readability

becksebenius-unity · 2022-01-19T17:09:06Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

+#if ENABLE_UNITY_COLLECTIONS_CHECKS
+                catch (InvalidOperationException e)
+                {
+                    //Can ignore, may happen if a pipeline is not used


Concerned about this - we ignore the exception when ENABLE_UNITY_COLLECTION_CHECKS is enabled, but when it's not enabled it will throw right? What happens when it throws?

This gets throwned only if this is defined.

Ohh I see. That's worth a comment I think to avoid others getting confused the same (if you keep the try/catch at least, I know simon has called it into question in another comment)

simon-lemay-unity · 2022-01-19T17:10:24Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

+                var pipeline = i == 0 ? m_UnreliableSequencedPipeline : m_ReliableSequencedFragmentedPipeline;
+
+#if ENABLE_UNITY_COLLECTIONS_CHECKS
+                try


Is the try-catch to ignore the exception really necessary? It seems to me the only way to raise InvalidOperationException from GetPipelineBuffers is if the connection is invalid. In the rest of the adapter we usually don't bother catching that one and assume our connections are always valid.

In my early test, it was. I have changed some stuff around, so might not be anymore. Will test.

simon-lemay-unity · 2022-01-19T17:13:15Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

+                {
+                    //Don't need to dispose of the buffers, they are filled with data pointers. 
+                    m_Driver.GetPipelineBuffers(pipeline, NetworkPipelineStageCollection.GetStageId(typeof(NetworkMetricsPipelineStage)),
+                        networkConnection, out var readProcessingBuffer, out var writeProcessingBuffer, out var sharedBuffer);


Minor, but you can explicitly ignore/discard some out parameters by passing out _.

0xFA11

generally looks good to me — I'd like to defer to Beck & Simon.

ian-m-unity · 2022-01-19T18:09:33Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

@@ -161,6 +161,7 @@ public struct ConnectionAddressData
        private NetworkPipeline m_UnreliableFragmentedPipeline;
        private NetworkPipeline m_UnreliableSequencedFragmentedPipeline;
        private NetworkPipeline m_ReliableSequencedFragmentedPipeline;
+        private const byte k_PipelineCount = 3; //Represented the number of existing pipeline


Any particular reason to use byte for this constant?

ian-m-unity · 2022-01-19T18:11:20Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

+            var networkConnection =  ParseClientId(transportClientId);
+
+            //Two pipelines, unreliable sequence and reliable fragmented
+            for (byte i = 0; i < k_PipelineCount; ++i)


Why are you using byte as the type of the loop variable? Most likely the machine doesn't have registers that small in any case, and this will just result in greater risk of overflow without reducing resource usage 🤔

I'll defer to @simon-lemay-unity for this. It does represent the small value well enough and I'm fine with it.

I don't really have any strong opinion on the matter. Usually it's advised to use register-sized variables for loop counters since that's what will be fastest. But here it's a loop with 3 iterations, so unlikely to have any effect. If only for consistency with other loops in the code I'd go with an int.

Still not sure I understand why we have a loop in the first place. Why isn't it just:

ProcessPipeline(m_UnreliableFragmentedPipeline); ProcessPipeline(m_UnreliableSequencedFragmentedPipeline); ProcessPipeline(m_ReliableSequencedFragmentedPipeline);

Because I forgot your comment and this makes a lot more sense.

ian-m-unity · 2022-01-19T18:12:50Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

+        private NetworkPipeline GetPipelineFromIndex(byte index)
+        {
+            switch (index)
+            {
+                case 0:
+                    return m_UnreliableFragmentedPipeline;
+                case 1:
+                    return m_UnreliableSequencedFragmentedPipeline;
+                case 2:
+                    return m_ReliableSequencedFragmentedPipeline;
+                default:
+                    throw new InvalidOperationException("Invalid Pipeline Index");
+            }
+        }


I think this would be better using an enum, rather than cases with the raw integer literals. I added a comment with an enum that could be used above on the k_PipelineCount constant.

ian-m-unity · 2022-01-19T18:16:09Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

@@ -161,6 +161,7 @@ public struct ConnectionAddressData
        private NetworkPipeline m_UnreliableFragmentedPipeline;
        private NetworkPipeline m_UnreliableSequencedFragmentedPipeline;
        private NetworkPipeline m_ReliableSequencedFragmentedPipeline;
+        private const byte k_PipelineCount = 3; //Represented the number of existing pipeline


I think it would be best to have an enum for this, rather than referring to these stages using raw numbers in the switch statement later. Something like:

enum PipelineType { UnreliableFragmented, UnreliableSequenceFragmented, ReliableSequenceFragmented, Count, }

This would replace count with (int)PipelineType.Count

Sure. I just wanted something simple in case things pipeline would change but you're right.

ian-m-unity · 2022-01-19T18:22:48Z

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

+                    //Connection 0 when host is self and bypass the transport so would be invalid, skip it.
+                    if (ngoConnectionId > 0 || (ngoConnectionId == 0 && !NetworkManager.Singleton.IsHost))
+                    {
+                        ExtractNetworkMetricsForClient(NetworkManager.Singleton.ClientIdToTransportId(ngoConnectionId));
+                    }


Rather than explaining the logic in a comment, you could write something like the following:

var isConnectionToSelf = ngoConnectionId == 0 && NetworkManager.Singleton.IsHost; if (isConnectionToSelf) { // Connection 0 when host is self bypasses the transport, so skip it. continue; } ExtractNetworkMetricsForClient(NetworkManager.Singleton.ClientIdToTransportId(ngoConnectionId));

This also reduces the complexity of the condition a bit.

ian-m-unity · 2022-01-19T18:27:07Z

com.unity.netcode.gameobjects/Runtime/Metrics/NetworkMetrics.cs

+        public void TrackPacketSent(uint packetCount)
+        {
+        }
+
+        public void TrackPacketReceived(uint packetCount)
+        {
+        }
+


Is this waiting for some changes in the tools package?

This will be part of the proper JIRA Ticket that dispatch it. With tests and all. It's there just to make sure we have functions.

responsability

First pass on moving data from pipeline stage to network metrics

b3484f2

Rosme requested review from simon-lemay-unity and a team January 13, 2022 20:13

simon-lemay-unity reviewed Jan 13, 2022

View reviewed changes

Extract metrics from network pipeline stage

2108fb1

Rosme changed the title ~~First pass on moving data from pipeline stage to network metrics~~ feat: Extract Metrics from Network Pipeline Stage Jan 19, 2022

Rosme marked this pull request as ready for review January 19, 2022 16:59

Rosme requested review from simon-lemay-unity and 0xFA11 January 19, 2022 16:59

simon-lemay-unity reviewed Jan 19, 2022

View reviewed changes

becksebenius-unity reviewed Jan 19, 2022

View reviewed changes

simon-lemay-unity reviewed Jan 19, 2022

View reviewed changes

0xFA11 reviewed Jan 19, 2022

View reviewed changes

JS Fauteux added 3 commits January 19, 2022 12:30

Updated based on comment, don't ignore exception anymore

1a25764

Merge branch 'develop' into feature/MTT-1855_metric-translation-layer

903232f

Added third pipeline to network metrics

183a0e4

becksebenius-unity approved these changes Jan 19, 2022

View reviewed changes

ian-m-unity reviewed Jan 19, 2022

View reviewed changes

JS Fauteux added 2 commits January 19, 2022 13:30

Followed comments: Avoid unneeded looping, better split of methods

e16a73c

responsability

Integrated changes from PR #1569 into this

50b01c3

simon-lemay-unity approved these changes Jan 19, 2022

View reviewed changes

simon-lemay-unity mentioned this pull request Jan 19, 2022

fix: Add metrics pipeline stage to unreliable pipeline #1569

Closed

Rosme enabled auto-merge (squash) January 19, 2022 18:50

Rosme merged commit e2dcebd into develop Jan 19, 2022

Rosme deleted the feature/MTT-1855_metric-translation-layer branch January 19, 2022 19:20

Rosme mentioned this pull request Feb 8, 2022

chore: backport multiple PRs as a single PR from experimental-develop branch #1665

Merged



		//TODO: Get the correct connection instead of m_ServerConnection
		m_Driver.GetPipelineBuffers(m_LastSendPipeline, NetworkPipelineStageCollection.GetStageId(typeof(NetworkMetricsPipelineStage)),

feat: Extract Metrics from Network Pipeline Stage #1583

feat: Extract Metrics from Network Pipeline Stage #1583

Uh oh!

Conversation

Rosme commented Jan 13, 2022

Testing and Documentation

Uh oh!

simon-lemay-unity commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

ian-m-unity Jan 19, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

ian-m-unity Jan 19, 2022 • 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 Jan 19, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simon-lemay-unity commented Jan 13, 2022 •

edited

Loading

ian-m-unity Jan 19, 2022 •

edited

Loading

ian-m-unity Jan 19, 2022 •

edited

Loading

ian-m-unity Jan 19, 2022 •

edited

Loading