Generate valid machine id #245
Conversation
There was a problem hiding this comment.
Copilot reviewed 10 out of 17 changed files in this pull request and generated no comments.
Files not reviewed (7)
- Runtime/Model/DataProvider.meta: Language not supported
- Runtime/Model/DataProvider/IMachineIdentifierDataProvider.cs.meta: Language not supported
- Runtime/Model/DataProvider/ISessionStorageDataProvider.cs.meta: Language not supported
- Runtime/Model/DataProvider/NetworkIdentifierDataProvider.cs.meta: Language not supported
- Runtime/Model/DataProvider/SessionStorageDataProvider.cs.meta: Language not supported
- Runtime/Model/DataProvider/UnityMachineIdentifierProvider.cs.meta: Language not supported
- Tests/Runtime/Client/Mocks/MachineIdStorageMock.cs.meta: Language not supported
Comments suppressed due to low confidence (4)
Runtime/Model/MachineIdStorage.cs:41
- Consider retrieving an existing machine ID from storage (possibly via FetchMachineIdFromStorage) before generating a new identifier to ensure consistency across sessions.
internal string GenerateMachineId()
Tests/Runtime/Client/BacktraceAttributeMachineIdTests.cs:28
- Correct the spelling of 'Netowrking' to 'Networking' in the test method name.
public void TestMachineAttributes_ShouldUseMac_ShouldReturnNetowrkingIdentifier()
Tests/Runtime/Client/BacktraceAttributeMachineIdTests.cs:76
- [nitpick] Consider correcting the method name to 'IdentifiersAreTheSame' for grammatical accuracy.
public void TestMachineAttributes_ShouldAlwaysReturnTheSameValueUnityId_IdentifierAreTheSame()
Tests/Runtime/Client/BacktraceAttributeMachineIdTests.cs:85
- [nitpick] Correct the spelling mistakes: 'Untiy' should be 'Unity' and 'Identitfier' should be 'Identifier'.
public void TestMachineAttributes_ShouldAlwaysGenerateTheSameUntiyAttribute_ShouldReturnTheSameUnityIdentitfier()
| /// <summary> | ||
| /// Converts a random string into a guid representation. | ||
| /// </summary> | ||
| public static Guid FromString(string value) |
There was a problem hiding this comment.
Can we rename this to FromMD5String? FromString suggests that a GUID string can be provided here.
There was a problem hiding this comment.
MD5 is an algorithm behind the scene that we're using to convert string into bytes that later we can use to generate GUID. It doesn't make sense to include MD5 in the name in my opinion. It's string and md5 is an implementation detail.
| [assembly: System.Runtime.CompilerServices.InternalsVisibleTo("Backtrace.Unity.Tests.Runtime")] | ||
| namespace Backtrace.Unity.Model.DataProvider | ||
| { | ||
| internal interface IMachineIdentifierDataProvider |
There was a problem hiding this comment.
This only provides the machine identifier. Why not name this IMachineIdentifierProvider?
| if (Guid.TryParse(_deviceUniqueIdentifier, out Guid unityUuidGuid)) | ||
| { | ||
| return unityUuidGuid.ToString(); | ||
| } | ||
| return GuidHelper.FromString(_deviceUniqueIdentifier).ToString(); |
There was a problem hiding this comment.
How often the ID will be generated? Maybe it's a good idea to generate this in the constructor, or cache the value somewhere?
There was a problem hiding this comment.
Oh, I see that in MachineIdStorage it is retrieved only once? If so, feel free to discard this comment.
There was a problem hiding this comment.
to confirm - yes. we're doing it only once.
Why
In the past, we used Unity ID to generate a user GUID attribute. If the Unity identifier was unavailable for any reason, we fall back to the network identifier. If both methods failed, we generated a random guid. Later, we also introduced storing the guid in the session memory.
Recently we noticed, the user identifier provided by Unity is not a valid guid. We thought it is based on the quick tests and results in Backtrace, but it looks like it's not on Windows at least.
To mitigate the issue:
Testing strategy:
Expected results:
ref: INT-93