Skip to content

fix: Metrics: Exception when tracking Named Messages [MTT-5380] #2426

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.

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
Mar 3, 2023

Conversation

samlucas-unity
Copy link
Contributor

This PR fixes an issue where Named Message Handlers could remove themselves causing an exception when the metrics tried to access the name of the message.

Metrics: Exception when tracking Named Messages in NGO [MTT-5380]
Github Issue: #2394

Changelog

  • Fixed: An issue where Named Message Handlers could remove themselves causing an exception when the metrics tried to access the name of the message

NetworkMetrics were tracking named message received events but it is possible for message handlers to unregister themselves.
We now cache the name of the message handler before invoking it so the Network Metrics can access the name even if its removed.
@samlucas-unity samlucas-unity requested a review from a team as a code owner February 27, 2023 15:09
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

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

Nice fix.
The last thing you need to do is add an update to the changlog file. Take a look at some of my pending PRs to get an idea of how to handle this while we are still in the middle of a release. It requires the existing changelog entries fall under a v1.3.0 entry and a new [Unreleased] section.
(it is easier to refrain from merging PRs until the release is finalized to avoid having to go back and pull out the PRs not included in the v1.3.0 release)

@samlucas-unity
Copy link
Contributor Author

Nice fix. The last thing you need to do is add an update to the changlog file. Take a look at some of my pending PRs to get an idea of how to handle this while we are still in the middle of a release. It requires the existing changelog entries fall under a v1.3.0 entry and a new [Unreleased] section. (it is easier to refrain from merging PRs until the release is finalized to avoid having to go back and pull out the PRs not included in the v1.3.0 release)

Thanks! I've updated the Changelog.

@samlucas-unity samlucas-unity changed the title MTT-5380 fix: MTT-5380 Metrics: Exception when tracking Named Messages in NGO Feb 28, 2023
@@ -36,6 +36,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
- Fixed `NetworkAnimator` issue with using pooled objects and when specific properties are cleaned during despawn and destroy.(#2309)
- Fixed issue where `NetworkAnimator` was checking for animation changes when the associated `NetworkObject` was not spawned.(#2309)
- Corrected an issue with the documentation for BufferSerializer (#2401)
- Fixed an issue where Named Message Handlers could remove themselves causing an exception when the metrics tried to access the name of the message.(#2394)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Fixed an issue where Named Message Handlers could remove themselves causing an exception when the metrics tried to access the name of the message.(#2394)
- Fixed an issue where Named Message Handlers could remove themselves causing an exception when the metrics tried to access the name of the message. (#2426)

The number at the end is not for GitHub Issue number, it's for the PR number :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cheers! Noel spotted it too, you beat me to it :)

@0xFA11 0xFA11 changed the title fix: MTT-5380 Metrics: Exception when tracking Named Messages in NGO fix: Metrics: Exception when tracking Named Messages [MTT-5380] Mar 1, 2023
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

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

Looks good to me!
(Made the minor changelog adjustment)

@NoelStephensUnity NoelStephensUnity merged commit e1fa36e into develop Mar 3, 2023
@NoelStephensUnity NoelStephensUnity deleted the MTT-5380 branch March 3, 2023 17:55
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