Skip to content

feat: Track scene event metrics#1089

Merged
josiemessa merged 13 commits intodevelopfrom
feat/MTT-748-scene-metrics
Aug 31, 2021
Merged

feat: Track scene event metrics#1089
josiemessa merged 13 commits intodevelopfrom
feat/MTT-748-scene-metrics

Conversation

@josiemessa
Copy link
Copy Markdown

Track when scene event messages are sent and received.

m_ServerLogReceivedEvent.Mark(new ServerLogEvent(new ConnectionInfo(senderClientId), (Unity.Multiplayer.MetricTypes.LogLevel)logType, bytesCount));
}

public void TrackSceneEventSent(ulong[] receiverClientIds, uint sceneEventType, string sceneName, long bytesCount)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will we have tests for this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes I've been digging into what I can do programmatically vs. need to do manually! I have manually verified they show up though, but next on the list is the tests.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You might be able to reuse the tests that were written by the SDK team if there are.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes that's what I'm looking at, as there's both programmatic and manual tests so need to see what's being tested in which.

foreach (var sceneIndex in SceneEventData.ScenesToSynchronize)
{
m_NetworkManager.NetworkMetrics.TrackSceneEventReceived(
clientId, (uint) SceneEventData.SceneEventType, GetSceneNameFromNetcodeSceneIndex(sceneIndex), stream.Length);
Copy link
Copy Markdown
Contributor

@becksebenius-unity becksebenius-unity Aug 27, 2021

Choose a reason for hiding this comment

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

I see why this was a point of discussion. In the profiler this might appear that stream.Length was sent for each scene, instead of the reality where stream.Length encompasses all scenes which are represented as separate events.

I'm wondering if maybe for this case it would be better to omit the scene name and include something like "N/A" instead of sending an event for all scenes? That would seem to be more accurate, although it would remove the utility of seeing which scenes were included - do you think there's a user need for that in this event?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah yeah that slipped my mind, do we have the same problem where we're doing this in other places (i.e. a message being sent about multiple things in one message but is split up for metrics)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah I remembered what the plan was - the idea was to reconstruct this into a "combined" metric on the formatting side (i.e. one metric event for all the scenes). Not ideal as the original metric is still reporting the combined size though. I also am not sure the best way to display this in the profiler either.

Copy link
Copy Markdown

@Rosme Rosme left a comment

Choose a reason for hiding this comment

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

I'd like a test, but looks good

m_ServerLogReceivedEvent.Mark(new ServerLogEvent(new ConnectionInfo(senderClientId), (Unity.Multiplayer.MetricTypes.LogLevel)logType, bytesCount));
}

public void TrackSceneEventSent(ulong[] receiverClientIds, uint sceneEventType, string sceneName, long bytesCount)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You might be able to reuse the tests that were written by the SDK team if there are.

@josiemessa josiemessa enabled auto-merge (squash) August 31, 2021 15:09
@josiemessa josiemessa merged commit 7fbc65c into develop Aug 31, 2021
@josiemessa josiemessa deleted the feat/MTT-748-scene-metrics branch August 31, 2021 15:25
SamuelBellomo added a commit that referenced this pull request Sep 2, 2021
…nsform

* develop: (26 commits)
  fix: client connected InvokeOnClientConnectedCallback with scene management disabled (#1123)
  fix: removed `public` class `NetcodeObserver` (MTT-1157) (#1122)
  feat: add NetworkMessageSent/Received metrics (#1112)
  feat: snapshot. MTU sizing option for Snapshot. MTT-1087 (#1111)
  Add metrics for transport bytes sent and received (#1104)
  fix: Missing end profiling sample (#1118)
  chore: support standalone mode for netcode runtimetests (#1115)
  feat: Change MetricNames for a more complex value type (#1109)
  feat: Track scene event metrics (#1089)
  style: whitespace fixes (#1117)
  feat: replace scene registration with scenes in build list (#1080)
  fix: mtt-857 GitHub issue 915 (#1099)
  fix: NetworkSceneManager exception when DontDestroyOnLoad NetworkObjects are being synchronized (#1090)
  feat: NetworkTransform Custom Editor Inspector UI (#1101)
  refactor: remove TempGlobalObjectIdHashOverride (#1105)
  fix: MTT-1124 Counters are now reported in sync with other metrics (#1096)
  refactor: convert using var statements to using var declarations (#1100)
  chore: updated all of the namespaces to match the tools package change (#1095)
  refactor!: remove network variable settings, network behaviour cleanup (#1097)
  fix: mtt-1088 review. Safer handling of out-of-order or old messages (#1091)
  ...

# Conflicts:
#	com.unity.netcode.gameobjects/Prototyping/NetworkTransform.cs
mollstam pushed a commit to Keepsake-Games/com.unity.netcode.gameobjects that referenced this pull request Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants