Skip to content
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

Fix: InstallationId now gets evaluated only once per application execution #3529

Merged
merged 8 commits into from
Aug 13, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Unable to load DLL sentry-native or one of its dependencies
- 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))
- Race condition in `SentryMessageHandler` ([#3477](https://github.com/getsentry/sentry-dotnet/pull/3477))
- Decrease runtime diagnostics circular buffer when profiling, reducing memory usage ([#3491](https://github.com/getsentry/sentry-dotnet/pull/3491))
- 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))

### Dependencies

Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,12 @@
using Sentry.Extensibility;
using Sentry.Internal.Extensions;

namespace Sentry;
namespace Sentry.Internal;

internal class InstallationIdHelper
internal class InstallationIdHelper(SentryOptions options)
jamescrosswell marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly object _installationIdLock = new();
private string? _installationId;
private readonly SentryOptions _options;
private readonly string? _persistenceDirectoryPath;

public InstallationIdHelper(SentryOptions options)
{
_options = options;
_persistenceDirectoryPath = options.CacheDirectoryPath ?? options.TryGetDsnSpecificCacheDirectoryPath();
jamescrosswell marked this conversation as resolved.
Show resolved Hide resolved
}

public string? TryGetInstallationId()
{
Expand Down Expand Up @@ -42,11 +34,11 @@ public InstallationIdHelper(SentryOptions options)

if (!string.IsNullOrWhiteSpace(id))
{
_options.LogDebug("Resolved installation ID '{0}'.", id);
options.LogDebug("Resolved installation ID '{0}'.", id);
}
else
{
_options.LogDebug("Failed to resolve installation ID.");
options.LogDebug("Failed to resolve installation ID.");
}

return _installationId = id;
Expand All @@ -57,13 +49,13 @@ public InstallationIdHelper(SentryOptions options)
{
try
{
var rootPath = _persistenceDirectoryPath ??
var rootPath = options.CacheDirectoryPath ??
Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData);
var directoryPath = Path.Combine(rootPath, "Sentry", _options.Dsn!.GetHashString());
var directoryPath = Path.Combine(rootPath, "Sentry", options.Dsn!.GetHashString());

Directory.CreateDirectory(directoryPath);

_options.LogDebug("Created directory for installation ID file ({0}).", directoryPath);
options.LogDebug("Created directory for installation ID file ({0}).", directoryPath);

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

Expand All @@ -72,20 +64,20 @@ public InstallationIdHelper(SentryOptions options)
{
return File.ReadAllText(filePath);
}
_options.LogDebug("File containing installation ID does not exist ({0}).", filePath);
options.LogDebug("File containing installation ID does not exist ({0}).", filePath);

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

_options.LogDebug("Saved installation ID '{0}' to file '{1}'.", id, filePath);
options.LogDebug("Saved installation ID '{0}' to file '{1}'.", id, filePath);
return id;
}
// If there's no write permission or the platform doesn't support this, we handle
// and let the next installation id strategy kick in
catch (Exception ex)
{
_options.LogError(ex, "Failed to resolve persistent installation ID.");
options.LogError(ex, "Failed to resolve persistent installation ID.");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do wonder if all three of these log statements need to be LogError. If we can't store a file with a generated id, it's not a huge problem as long as we can read an machine identifier from the network card.

It feels like this would be an error only if none of the 3 techniques we try to resolve an installation id are successful.

return null;
}
}
Expand All @@ -105,15 +97,15 @@ public InstallationIdHelper(SentryOptions options)

if (string.IsNullOrWhiteSpace(installationId))
{
_options.LogError("Failed to find an appropriate network interface for installation ID.");
options.LogError("Failed to find an appropriate network interface for installation ID.");
return null;
}

return installationId;
}
catch (Exception ex)
{
_options.LogError(ex, "Failed to resolve hardware installation ID.");
options.LogError(ex, "Failed to resolve hardware installation ID.");
return null;
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/Sentry/SentryOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,8 @@ public class SentryOptions
/// </remarks>
internal IScopeStackContainer? ScopeStackContainer { get; set; }

private Lazy<string?> LazyInstallationId => new(()
=> new InstallationIdHelper(this).TryGetInstallationId());
internal string? InstallationId => LazyInstallationId.Value;
private readonly Lazy<string?> _lazyInstallationId;
internal string? InstallationId => _lazyInstallationId.Value;

#if __MOBILE__
private bool _isGlobalModeEnabled = true;
Expand Down Expand Up @@ -1182,6 +1181,7 @@ public bool JsonPreserveReferences
public SentryOptions()
{
SettingLocator = new SettingLocator(this);
_lazyInstallationId = new(() => new InstallationIdHelper(this).TryGetInstallationId());
bitsandfoxes marked this conversation as resolved.
Show resolved Hide resolved

TransactionProcessorsProviders = new() {
() => TransactionProcessors ?? Enumerable.Empty<ISentryTransactionProcessor>()
Expand Down
35 changes: 27 additions & 8 deletions test/Sentry.Tests/Internals/InstallationIdHelperTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ private class Fixture : IDisposable
{
private readonly TempDirectory _cacheDirectory;

public InMemoryDiagnosticLogger Logger { get; }
public IDiagnosticLogger Logger { get; }

public SentryOptions Options { get; }

public Fixture(Action<SentryOptions> configureOptions = null)
{
Logger = new InMemoryDiagnosticLogger();
Logger = Substitute.For<IDiagnosticLogger>();

var fileSystem = new FakeFileSystem();
_cacheDirectory = new TempDirectory(fileSystem);
Expand Down Expand Up @@ -40,9 +40,6 @@ public Fixture(Action<SentryOptions> configureOptions = null)
[Fact]
public void GetMachineNameInstallationId_Hashed()
{
// Arrange
var sut = _fixture.GetSut();

// Act
var installationId = InstallationIdHelper.GetMachineNameInstallationId();

Expand All @@ -54,9 +51,6 @@ public void GetMachineNameInstallationId_Hashed()
[Fact]
public void GetMachineNameInstallationId_Idempotent()
{
// Arrange
var sut = _fixture.GetSut();

// Act
var installationIds = Enumerable
.Range(0, 10)
Expand All @@ -66,4 +60,29 @@ public void GetMachineNameInstallationId_Idempotent()
// Assert
installationIds.Distinct().Should().ContainSingle();
}

[Fact]
public void TryGetInstallationId_CachesInstallationId()
{
// Arrange
_fixture.Logger.IsEnabled(Arg.Any<SentryLevel>()).Returns(true);
var installationIdHelper = _fixture.GetSut();

// Act
var installationId1 = installationIdHelper.TryGetInstallationId();

// Assert
installationId1.Should().NotBeNullOrWhiteSpace();
_fixture.Logger.Received(1).Log(SentryLevel.Debug, "Resolved installation ID '{0}'.", null, Arg.Any<string>());

// Arrange
_fixture.Logger.ClearReceivedCalls();

// Act
var installationId2 = installationIdHelper.TryGetInstallationId();

// Assert
installationId2.Should().Be(installationId1);
_fixture.Logger.Received(0).Log(SentryLevel.Debug, "Resolved installation ID '{0}'.", null, Arg.Any<string>());
}
}
30 changes: 30 additions & 0 deletions test/Sentry.Tests/SentryOptionsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -738,4 +738,34 @@ public void Integrations_Includes_MajorSystemPrefixes(string expected)
var sut = new SentryOptions();
Assert.Contains(sut.InAppExclude!, e => e.ToString() == expected);
}

[Fact]
public void CachesInstallationId()
{
// Arrange
var logger = Substitute.For<IDiagnosticLogger>();
logger.IsEnabled(Arg.Any<SentryLevel>()).Returns(true);
var sut = new SentryOptions
{
Debug = true,
DiagnosticLogger = logger
};

// Act
var installationId1 = sut.InstallationId;

// Assert
installationId1.Should().NotBeNullOrWhiteSpace();
logger.Received(1).Log(SentryLevel.Debug, "Resolved installation ID '{0}'.", null, Arg.Any<string>());

// Arrange
logger.ClearReceivedCalls();

// Act
var installationId2 = sut.InstallationId;

// Assert
installationId2.Should().Be(installationId1);
logger.Received(0).Log(SentryLevel.Debug, "Resolved installation ID '{0}'.", null, Arg.Any<string>());
}
}