Skip to content

Commit b2d6162

Browse files
jamescrosswelljamescrosswell
authored
Fix: InstallationId now gets evaluated only once per application execution (#3529)
1 parent 5d00295 commit b2d6162

File tree

5 files changed

+73
-31
lines changed

5 files changed

+73
-31
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ Unable to load DLL sentry-native or one of its dependencies
1414
- On mobile devices, the SDK no longer throws a `FormatException` when trying to report native events ([#3485](https://github.com/getsentry/sentry-dotnet/pull/3485))
1515
- Race condition in `SentryMessageHandler` ([#3477](https://github.com/getsentry/sentry-dotnet/pull/3477))
1616
- Decrease runtime diagnostics circular buffer when profiling, reducing memory usage ([#3491](https://github.com/getsentry/sentry-dotnet/pull/3491))
17+
- The InstallationId is now resolved only once per application execution and any issues are logged as warnings instead of errors ([#3529](https://github.com/getsentry/sentry-dotnet/pull/3529))
1718
- DisplayInfo now captured correctly on iOS and Mac Catalyst on non-UI threads ([#3521](https://github.com/getsentry/sentry-dotnet/pull/3521))
1819

1920
### Dependencies

‎src/Sentry/InstallationIdHelper.cs‎ renamed to ‎src/Sentry/Internal/InstallationIdHelper.cs‎

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,12 @@
11
using Sentry.Extensibility;
22
using Sentry.Internal.Extensions;
33

4-
namespace Sentry;
4+
namespace Sentry.Internal;
55

6-
internal class InstallationIdHelper
6+
internal class InstallationIdHelper(SentryOptions options)
77
{
88
private readonly object _installationIdLock = new();
99
private string? _installationId;
10-
private readonly SentryOptions _options;
11-
private readonly string? _persistenceDirectoryPath;
12-
13-
public InstallationIdHelper(SentryOptions options)
14-
{
15-
_options = options;
16-
_persistenceDirectoryPath = options.CacheDirectoryPath ?? options.TryGetDsnSpecificCacheDirectoryPath();
17-
}
1810

1911
public string? TryGetInstallationId()
2012
{
@@ -42,11 +34,11 @@ public InstallationIdHelper(SentryOptions options)
4234

4335
if (!string.IsNullOrWhiteSpace(id))
4436
{
45-
_options.LogDebug("Resolved installation ID '{0}'.", id);
37+
options.LogDebug("Resolved installation ID '{0}'.", id);
4638
}
4739
else
4840
{
49-
_options.LogDebug("Failed to resolve installation ID.");
41+
options.LogDebug("Failed to resolve installation ID.");
5042
}
5143

5244
return _installationId = id;
@@ -57,13 +49,13 @@ public InstallationIdHelper(SentryOptions options)
5749
{
5850
try
5951
{
60-
var rootPath = _persistenceDirectoryPath ??
52+
var rootPath = options.CacheDirectoryPath ??
6153
Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData);
62-
var directoryPath = Path.Combine(rootPath, "Sentry", _options.Dsn!.GetHashString());
54+
var directoryPath = Path.Combine(rootPath, "Sentry", options.Dsn!.GetHashString());
6355

6456
Directory.CreateDirectory(directoryPath);
6557

66-
_options.LogDebug("Created directory for installation ID file ({0}).", directoryPath);
58+
options.LogDebug("Created directory for installation ID file ({0}).", directoryPath);
6759

6860
var filePath = Path.Combine(directoryPath, ".installation");
6961

@@ -72,20 +64,20 @@ public InstallationIdHelper(SentryOptions options)
7264
{
7365
return File.ReadAllText(filePath);
7466
}
75-
_options.LogDebug("File containing installation ID does not exist ({0}).", filePath);
67+
options.LogDebug("File containing installation ID does not exist ({0}).", filePath);
7668

7769
// Generate new installation ID and store it in a file
7870
var id = Guid.NewGuid().ToString();
7971
File.WriteAllText(filePath, id);
8072

81-
_options.LogDebug("Saved installation ID '{0}' to file '{1}'.", id, filePath);
73+
options.LogDebug("Saved installation ID '{0}' to file '{1}'.", id, filePath);
8274
return id;
8375
}
8476
// If there's no write permission or the platform doesn't support this, we handle
8577
// and let the next installation id strategy kick in
8678
catch (Exception ex)
8779
{
88-
_options.LogError(ex, "Failed to resolve persistent installation ID.");
80+
options.LogError(ex, "Failed to resolve persistent installation ID.");
8981
return null;
9082
}
9183
}
@@ -105,15 +97,15 @@ public InstallationIdHelper(SentryOptions options)
10597

10698
if (string.IsNullOrWhiteSpace(installationId))
10799
{
108-
_options.LogError("Failed to find an appropriate network interface for installation ID.");
100+
options.LogError("Failed to find an appropriate network interface for installation ID.");
109101
return null;
110102
}
111103

112104
return installationId;
113105
}
114106
catch (Exception ex)
115107
{
116-
_options.LogError(ex, "Failed to resolve hardware installation ID.");
108+
options.LogError(ex, "Failed to resolve hardware installation ID.");
117109
return null;
118110
}
119111
}

‎src/Sentry/SentryOptions.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,8 @@ public class SentryOptions
4141
/// </remarks>
4242
internal IScopeStackContainer? ScopeStackContainer { get; set; }
4343

44-
private Lazy<string?> LazyInstallationId => new(()
45-
=> new InstallationIdHelper(this).TryGetInstallationId());
46-
internal string? InstallationId => LazyInstallationId.Value;
44+
private readonly Lazy<string?> _lazyInstallationId;
45+
internal string? InstallationId => _lazyInstallationId.Value;
4746

4847
#if __MOBILE__
4948
private bool _isGlobalModeEnabled = true;
@@ -1182,6 +1181,7 @@ public bool JsonPreserveReferences
11821181
public SentryOptions()
11831182
{
11841183
SettingLocator = new SettingLocator(this);
1184+
_lazyInstallationId = new(() => new InstallationIdHelper(this).TryGetInstallationId());
11851185

11861186
TransactionProcessorsProviders = new() {
11871187
() => TransactionProcessors ?? Enumerable.Empty<ISentryTransactionProcessor>()

‎test/Sentry.Tests/Internals/InstallationIdHelperTests.cs‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,13 @@ private class Fixture : IDisposable
66
{
77
private readonly TempDirectory _cacheDirectory;
88

9-
public InMemoryDiagnosticLogger Logger { get; }
9+
public IDiagnosticLogger Logger { get; }
1010

1111
public SentryOptions Options { get; }
1212

1313
public Fixture(Action<SentryOptions> configureOptions = null)
1414
{
15-
Logger = new InMemoryDiagnosticLogger();
15+
Logger = Substitute.For<IDiagnosticLogger>();
1616

1717
var fileSystem = new FakeFileSystem();
1818
_cacheDirectory = new TempDirectory(fileSystem);
@@ -40,9 +40,6 @@ public Fixture(Action<SentryOptions> configureOptions = null)
4040
[Fact]
4141
public void GetMachineNameInstallationId_Hashed()
4242
{
43-
// Arrange
44-
var sut = _fixture.GetSut();
45-
4643
// Act
4744
var installationId = InstallationIdHelper.GetMachineNameInstallationId();
4845

@@ -54,9 +51,6 @@ public void GetMachineNameInstallationId_Hashed()
5451
[Fact]
5552
public void GetMachineNameInstallationId_Idempotent()
5653
{
57-
// Arrange
58-
var sut = _fixture.GetSut();
59-
6054
// Act
6155
var installationIds = Enumerable
6256
.Range(0, 10)
@@ -66,4 +60,29 @@ public void GetMachineNameInstallationId_Idempotent()
6660
// Assert
6761
installationIds.Distinct().Should().ContainSingle();
6862
}
63+
64+
[Fact]
65+
public void TryGetInstallationId_CachesInstallationId()
66+
{
67+
// Arrange
68+
_fixture.Logger.IsEnabled(Arg.Any<SentryLevel>()).Returns(true);
69+
var installationIdHelper = _fixture.GetSut();
70+
71+
// Act
72+
var installationId1 = installationIdHelper.TryGetInstallationId();
73+
74+
// Assert
75+
installationId1.Should().NotBeNullOrWhiteSpace();
76+
_fixture.Logger.Received(1).Log(SentryLevel.Debug, "Resolved installation ID '{0}'.", null, Arg.Any<string>());
77+
78+
// Arrange
79+
_fixture.Logger.ClearReceivedCalls();
80+
81+
// Act
82+
var installationId2 = installationIdHelper.TryGetInstallationId();
83+
84+
// Assert
85+
installationId2.Should().Be(installationId1);
86+
_fixture.Logger.Received(0).Log(SentryLevel.Debug, "Resolved installation ID '{0}'.", null, Arg.Any<string>());
87+
}
6988
}

‎test/Sentry.Tests/SentryOptionsTests.cs‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -738,4 +738,34 @@ public void Integrations_Includes_MajorSystemPrefixes(string expected)
738738
var sut = new SentryOptions();
739739
Assert.Contains(sut.InAppExclude!, e => e.ToString() == expected);
740740
}
741+
742+
[Fact]
743+
public void CachesInstallationId()
744+
{
745+
// Arrange
746+
var logger = Substitute.For<IDiagnosticLogger>();
747+
logger.IsEnabled(Arg.Any<SentryLevel>()).Returns(true);
748+
var sut = new SentryOptions
749+
{
750+
Debug = true,
751+
DiagnosticLogger = logger
752+
};
753+
754+
// Act
755+
var installationId1 = sut.InstallationId;
756+
757+
// Assert
758+
installationId1.Should().NotBeNullOrWhiteSpace();
759+
logger.Received(1).Log(SentryLevel.Debug, "Resolved installation ID '{0}'.", null, Arg.Any<string>());
760+
761+
// Arrange
762+
logger.ClearReceivedCalls();
763+
764+
// Act
765+
var installationId2 = sut.InstallationId;
766+
767+
// Assert
768+
installationId2.Should().Be(installationId1);
769+
logger.Received(0).Log(SentryLevel.Debug, "Resolved installation ID '{0}'.", null, Arg.Any<string>());
770+
}
741771
}

0 commit comments

Comments
 (0)