From b052192cf2fc591f8820342032cd286bf9f2dfeb Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Mon, 7 Oct 2024 12:54:38 +0200 Subject: [PATCH 1/5] feat: options.AddProfilingIntegration() --- .../Program.cs | 2 +- .../Program.cs | 2 +- src/Sentry.Profiling/ProfilingIntegration.cs | 4 + .../SentryOptionsProfilingExtensions.cs | 44 +++++++++++ src/Sentry/SentrySdk.cs | 2 +- .../ProfilingSentryOptionsExtensionsTests.cs | 76 +++++++++++++++++++ .../Resources/README.md | 2 +- .../SamplingTransactionProfilerTests.cs | 10 +-- 8 files changed, 133 insertions(+), 9 deletions(-) create mode 100644 src/Sentry.Profiling/SentryOptionsProfilingExtensions.cs create mode 100644 test/Sentry.Profiling.Tests/ProfilingSentryOptionsExtensionsTests.cs diff --git a/samples/Sentry.Samples.AspNetCore.WebAPI.Profiling/Program.cs b/samples/Sentry.Samples.AspNetCore.WebAPI.Profiling/Program.cs index dde356bb5d..07b0147102 100644 --- a/samples/Sentry.Samples.AspNetCore.WebAPI.Profiling/Program.cs +++ b/samples/Sentry.Samples.AspNetCore.WebAPI.Profiling/Program.cs @@ -3,7 +3,7 @@ builder.WebHost.UseSentry(o => { o.Dsn = "https://eb18e953812b41c3aeb042e666fd3b5c@o447951.ingest.sentry.io/5428537"; - o.AddIntegration(new ProfilingIntegration()); + o.AddProfilingIntegration(); o.ProfilesSampleRate = 0.1; o.TracesSampleRate = 1.0; }); diff --git a/samples/Sentry.Samples.Console.Profiling/Program.cs b/samples/Sentry.Samples.Console.Profiling/Program.cs index bea41d8756..7c0b1528f4 100644 --- a/samples/Sentry.Samples.Console.Profiling/Program.cs +++ b/samples/Sentry.Samples.Console.Profiling/Program.cs @@ -22,7 +22,7 @@ private static void Main() // Debugging options.ShutdownTimeout = TimeSpan.FromMinutes(5); - options.AddIntegration(new ProfilingIntegration(TimeSpan.FromMilliseconds(500))); + options.AddProfilingIntegration(TimeSpan.FromMilliseconds(500)); })) { var tx = SentrySdk.StartTransaction("app", "run"); diff --git a/src/Sentry.Profiling/ProfilingIntegration.cs b/src/Sentry.Profiling/ProfilingIntegration.cs index d88eee852e..195dfa9dc8 100644 --- a/src/Sentry.Profiling/ProfilingIntegration.cs +++ b/src/Sentry.Profiling/ProfilingIntegration.cs @@ -42,5 +42,9 @@ public void Register(IHub hub, SentryOptions options) options.LogError(e, "Failed to initialize the profiler"); } } + else + { + options.LogInfo("Profiling Integration is disabled because profiling is disabled by configuration."); + } } } diff --git a/src/Sentry.Profiling/SentryOptionsProfilingExtensions.cs b/src/Sentry.Profiling/SentryOptionsProfilingExtensions.cs new file mode 100644 index 0000000000..2988912400 --- /dev/null +++ b/src/Sentry.Profiling/SentryOptionsProfilingExtensions.cs @@ -0,0 +1,44 @@ +using Sentry.Extensibility; +using Sentry.Profiling; + +namespace Sentry; + +/// +/// The additional Sentry Options extensions from Sentry Profiling. +/// +[EditorBrowsable(EditorBrowsableState.Never)] +public static class SentryOptionsProfilingExtensions +{ + /// + /// Adds ProfilingIntegration to Sentry. + /// + /// The Sentry options. + /// + /// If not given or TimeSpan.Zero, then the profiler initialization is asynchronous. + /// This is useful for applications that need to start quickly. The profiler will start in the background + /// and will be ready to capture transactions that have started after the profiler has started. + /// + /// If given a non-zero timeout, profiling startup blocks up to the given amount of time. If the timeout is reached + /// and the profiler session hasn't started yet, the execution is unblocked and behaves as the async startup, + /// i.e. transactions will be profiled only after the session is eventually started. + /// + public static void AddProfilingIntegration(this SentryOptions options, TimeSpan startupTimeout = default) + { + if (options.HasIntegration()) + { + options.LogWarning($"{nameof(ProfilingIntegration)} has already been added. The second call to {nameof(AddProfilingIntegration)} will be ignored."); + return; + } + + options.AddIntegration(new ProfilingIntegration(startupTimeout)); + } + + /// + /// Disables the Profiling integration. + /// + /// The SentryOptions to remove the integration from. + public static void DisableProfilingIntegration(this SentryOptions options) + { + options.RemoveIntegration(); + } +} diff --git a/src/Sentry/SentrySdk.cs b/src/Sentry/SentrySdk.cs index bf32c20d6c..e3d6abb8f0 100644 --- a/src/Sentry/SentrySdk.cs +++ b/src/Sentry/SentrySdk.cs @@ -102,7 +102,7 @@ internal static IHub InitHub(SentryOptions options) #endif { LogWarningIfProfilingMisconfigured(options, ", because ProfilingIntegration from package Sentry.Profiling" + - " hasn't been registered. You can do that by calling 'options.AddIntegration(new ProfilingIntegration())'"); + " hasn't been registered. You can do that by calling 'options.AddProfilingIntegration()'"); } #endif diff --git a/test/Sentry.Profiling.Tests/ProfilingSentryOptionsExtensionsTests.cs b/test/Sentry.Profiling.Tests/ProfilingSentryOptionsExtensionsTests.cs new file mode 100644 index 0000000000..5e1c160d9f --- /dev/null +++ b/test/Sentry.Profiling.Tests/ProfilingSentryOptionsExtensionsTests.cs @@ -0,0 +1,76 @@ +namespace Sentry.Profiling.Tests; + +#nullable enable + +[UsesVerify] + +public class ProfilingSentryOptionsExtensionsTests +{ + private readonly InMemoryDiagnosticLogger _logger = new(); + private readonly SentryOptions _options = new() + { + Dsn = ValidDsn, + AutoSessionTracking = false, + IsGlobalModeEnabled = true, + BackgroundWorker = Substitute.For(), + Debug = true, + + // Set explicitly for this test in case the defaults change in the future. + TracesSampleRate = 0.0, + TracesSampler = null + }; + + public ProfilingSentryOptionsExtensionsTests() + { + _options.DiagnosticLogger = _logger; + _options.AddProfilingIntegration(); + } + + private Hub GetSut() => new(_options, Substitute.For()); + + private static IEnumerable GetIntegrations(ISentryClient hub) => + hub.GetSentryOptions()?.Integrations ?? Enumerable.Empty(); + + [Fact] + public void Integration_DisabledWithDefaultOptions() + { + using var hub = GetSut(); + var integrations = GetIntegrations(hub); + Assert.Contains(_logger.Entries, x => x.Message == "Profiling Integration is disabled because profiling is disabled by configuration." + && x.Level == SentryLevel.Info); + } + + [Fact] + public void Integration_EnabledBySampleRate() + { + _options.TracesSampleRate = 1.0; + _options.ProfilesSampleRate = 1.0; + + using var hub = GetSut(); + var integrations = GetIntegrations(hub); + Assert.Contains(integrations, i => i is ProfilingIntegration); + } + + [Fact] + public void DisableProfilingIntegration_RemovesProfilingIntegration() + { + _options.TracesSampleRate = 1.0; + _options.ProfilesSampleRate = 1.0; + _options.DisableProfilingIntegration(); + + using var hub = GetSut(); + var integrations = GetIntegrations(hub); + Assert.DoesNotContain(integrations, i => i is ProfilingIntegration); + } + + [Fact] + public void AddProfilingIntegration_DoesntDuplicate() + { + var options = new SentryOptions(); + + options.AddProfilingIntegration(); + options.AddProfilingIntegration(); + + Assert.Single(options.Integrations, x => x is ProfilingIntegration); + } +} diff --git a/test/Sentry.Profiling.Tests/Resources/README.md b/test/Sentry.Profiling.Tests/Resources/README.md index 9d90debc08..db5693a9ee 100644 --- a/test/Sentry.Profiling.Tests/Resources/README.md +++ b/test/Sentry.Profiling.Tests/Resources/README.md @@ -22,7 +22,7 @@ public static int Main(string[] args) o.Dsn = DefaultDsn; o.Debug = true; o.TracesSampleRate = 1.0; - o.AddIntegration(new ProfilingIntegration(Path.GetTempPath())); + o.AddProfilingIntegration(); o.DiagnosticLogger = new FileAppenderDiagnosticLogger("C:/dev/Aura.UI/test.log", SentryLevel.Debug); })) { diff --git a/test/Sentry.Profiling.Tests/SamplingTransactionProfilerTests.cs b/test/Sentry.Profiling.Tests/SamplingTransactionProfilerTests.cs index 8d42e1e13a..a8ab401741 100644 --- a/test/Sentry.Profiling.Tests/SamplingTransactionProfilerTests.cs +++ b/test/Sentry.Profiling.Tests/SamplingTransactionProfilerTests.cs @@ -224,7 +224,7 @@ async Task VerifyAsync(HttpRequestMessage message) // Disable process exit flush to resolve "There is no currently active test." errors. options.DisableAppDomainProcessExitFlush(); - options.AddIntegration(new ProfilingIntegration(TimeSpan.FromSeconds(10))); + options.AddProfilingIntegration(TimeSpan.FromSeconds(10)); try { @@ -314,7 +314,7 @@ async Task VerifyAsync(HttpRequestMessage message) ProfilesSampleRate = 1.0, }; - options.AddIntegration(new ProfilingIntegration(TimeSpan.FromSeconds(10))); + options.AddProfilingIntegration(TimeSpan.FromSeconds(10)); try { @@ -361,7 +361,7 @@ public void ProfilerIntegration_WithProfilingDisabled_LeavesFactoryNull() TracesSampleRate = 1.0, ProfilesSampleRate = 0, }; - options.AddIntegration(new ProfilingIntegration()); + options.AddProfilingIntegration(); using var hub = new Hub(options); Assert.Null(hub.Options.TransactionProfilerFactory); } @@ -375,7 +375,7 @@ public void ProfilerIntegration_WithTracingDisabled_LeavesFactoryNull() TracesSampleRate = 0, ProfilesSampleRate = 1.0, }; - options.AddIntegration(new ProfilingIntegration()); + options.AddProfilingIntegration(); using var hub = new Hub(options); Assert.Null(hub.Options.TransactionProfilerFactory); } @@ -389,7 +389,7 @@ public void ProfilerIntegration_WithProfilingEnabled_SetsFactory() TracesSampleRate = 1.0, ProfilesSampleRate = 1.0, }; - options.AddIntegration(new ProfilingIntegration()); + options.AddProfilingIntegration(); using var hub = new Hub(options); Assert.NotNull(hub.Options.TransactionProfilerFactory); } From a661e221325f356415b34266bb283735ed61b985 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Mon, 7 Oct 2024 12:56:00 +0200 Subject: [PATCH 2/5] chore: update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c64b9c765..770b62642d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Features + +- Added `SentryOptions` extension for profiling `options.AddProfilingIntegration()` ([#3660](https://github.com/getsentry/sentry-dotnet/pull/3660)) + ### Dependencies - Bump CLI from v2.36.5 to v2.36.6 ([#3647](https://github.com/getsentry/sentry-dotnet/pull/3647)) From 435b93e64cfbbaf9f2a6a5d0149f3d6f3c04e989 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Tue, 12 Nov 2024 09:36:31 +0100 Subject: [PATCH 3/5] chore: update changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 28b477b16c..b0702fce9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,8 @@ ### Features -- Added `SentryOptions` extension for profiling `options.AddProfilingIntegration()` ([#3660](https://github.com/getsentry/sentry-dotnet/pull/3660)) +- Added `SentryOptions` extension for profiling: `options.AddProfilingIntegration()` ([#3660](https://github.com/getsentry/sentry-dotnet/pull/3660)) + ### Fixes - Fixed ArgumentNullException in FormRequestPayloadExtractor when handling invalid form data on ASP.NET ([#3734](https://github.com/getsentry/sentry-dotnet/pull/3734)) From 5abd66de4a5498bf9c5b3bec24580c96ef346053 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Sun, 17 Nov 2024 21:26:20 +0100 Subject: [PATCH 4/5] chore: add ProfilingIntegration to cocoa --- .../Platforms/Cocoa/ProfilingIntegration.cs | 31 ++++++++++++++++ src/Sentry/Platforms/Cocoa/SentryOptions.cs | 36 +++++++++++++++++++ src/Sentry/Platforms/Cocoa/SentrySdk.cs | 8 ++--- 3 files changed, 69 insertions(+), 6 deletions(-) create mode 100644 src/Sentry/Platforms/Cocoa/ProfilingIntegration.cs diff --git a/src/Sentry/Platforms/Cocoa/ProfilingIntegration.cs b/src/Sentry/Platforms/Cocoa/ProfilingIntegration.cs new file mode 100644 index 0000000000..9114ac6c0d --- /dev/null +++ b/src/Sentry/Platforms/Cocoa/ProfilingIntegration.cs @@ -0,0 +1,31 @@ +using Sentry.Extensibility; +using Sentry.Integrations; + +namespace Sentry.Cocoa; + +/// +/// Enables transaction performance profiling. +/// +public class ProfilingIntegration : ISdkIntegration +{ + /// + public void Register(IHub hub, SentryOptions options) + { + if (options.IsProfilingEnabled) + { + try + { + options.LogDebug("Profiling is enabled, attaching native SDK profiler factory"); + options.TransactionProfilerFactory ??= new CocoaProfilerFactory(options); + } + catch (Exception e) + { + options.LogError(e, "Failed to initialize the profiler"); + } + } + else + { + options.LogInfo("Profiling Integration is disabled because profiling is disabled by configuration."); + } + } +} diff --git a/src/Sentry/Platforms/Cocoa/SentryOptions.cs b/src/Sentry/Platforms/Cocoa/SentryOptions.cs index c14213087a..ae28a38afb 100644 --- a/src/Sentry/Platforms/Cocoa/SentryOptions.cs +++ b/src/Sentry/Platforms/Cocoa/SentryOptions.cs @@ -1,4 +1,6 @@ using ObjCRuntime; +using Sentry.Cocoa; +using Sentry.Extensibility; // ReSharper disable once CheckNamespace namespace Sentry; @@ -215,4 +217,38 @@ public void AddInAppInclude(string prefix) InAppIncludes.Add(prefix); } } + + // We actually add the profiling integration automatically in InitSentryCocoaSdk(). + // However, if user calls AddProfilingIntegration() multiple times, we print a warning, as usual. + private bool _profilingIntegrationAddedByUser = false; + + /// + /// Adds ProfilingIntegration to Sentry. + /// + /// + /// Unused, only here so that the signature is the same as AddProfilingIntegration() from package Sentry.Profiling. + /// + public void AddProfilingIntegration(TimeSpan startupTimeout = default) + { + if (HasIntegration()) + { + if (_profilingIntegrationAddedByUser) + { + DiagnosticLogger?.LogWarning($"{nameof(ProfilingIntegration)} has already been added. The second call to {nameof(AddProfilingIntegration)} will be ignored."); + } + return; + } + + _profilingIntegrationAddedByUser = true; + AddIntegration(new ProfilingIntegration()); + } + + /// + /// Disables the Profiling integration. + /// + public void DisableProfilingIntegration() + { + _profilingIntegrationAddedByUser = false; + RemoveIntegration(); + } } diff --git a/src/Sentry/Platforms/Cocoa/SentrySdk.cs b/src/Sentry/Platforms/Cocoa/SentrySdk.cs index ac075af7a6..052fbf9025 100644 --- a/src/Sentry/Platforms/Cocoa/SentrySdk.cs +++ b/src/Sentry/Platforms/Cocoa/SentrySdk.cs @@ -192,12 +192,8 @@ private static void InitSentryCocoaSdk(SentryOptions options) options.EnableScopeSync = true; options.ScopeObserver = new CocoaScopeObserver(options); - if (options.IsProfilingEnabled) - { - options.LogDebug("Profiling is enabled, attaching native SDK profiler factory"); - options.TransactionProfilerFactory ??= new CocoaProfilerFactory(options); - } - + // Note: don't use AddProfilingIntegration as it would print a warning if user used it too. + options.AddIntegration(new ProfilingIntegration()); // TODO: Pause/Resume } From 24e6bde930d66ef2c6d8ea26e4845b28fa3a13c4 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Fri, 22 Nov 2024 08:10:11 +0100 Subject: [PATCH 5/5] check prior to adding the default integration --- src/Sentry/Platforms/Cocoa/SentrySdk.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Sentry/Platforms/Cocoa/SentrySdk.cs b/src/Sentry/Platforms/Cocoa/SentrySdk.cs index 052fbf9025..b56ce3ba26 100644 --- a/src/Sentry/Platforms/Cocoa/SentrySdk.cs +++ b/src/Sentry/Platforms/Cocoa/SentrySdk.cs @@ -193,7 +193,10 @@ private static void InitSentryCocoaSdk(SentryOptions options) options.ScopeObserver = new CocoaScopeObserver(options); // Note: don't use AddProfilingIntegration as it would print a warning if user used it too. - options.AddIntegration(new ProfilingIntegration()); + if (!options.HasIntegration()) + { + options.AddIntegration(new ProfilingIntegration()); + } // TODO: Pause/Resume }