-
-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: options.AddProfilingIntegration() #3660
Changes from 7 commits
b052192
a661e22
cc06c9e
9f02c02
2ae2700
435b93e
5abd66d
24e6bde
bd53758
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
using Sentry.Extensibility; | ||
using Sentry.Profiling; | ||
|
||
namespace Sentry; | ||
|
||
/// <summary> | ||
/// The additional Sentry Options extensions from Sentry Profiling. | ||
/// </summary> | ||
[EditorBrowsable(EditorBrowsableState.Never)] | ||
public static class SentryOptionsProfilingExtensions | ||
{ | ||
/// <summary> | ||
/// Adds ProfilingIntegration to Sentry. | ||
/// </summary> | ||
/// <param name="options">The Sentry options.</param> | ||
/// <param name="startupTimeout"> | ||
/// 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. | ||
/// </param> | ||
public static void AddProfilingIntegration(this SentryOptions options, TimeSpan startupTimeout = default) | ||
{ | ||
if (options.HasIntegration<ProfilingIntegration>()) | ||
{ | ||
options.LogWarning($"{nameof(ProfilingIntegration)} has already been added. The second call to {nameof(AddProfilingIntegration)} will be ignored."); | ||
return; | ||
} | ||
|
||
options.AddIntegration(new ProfilingIntegration(startupTimeout)); | ||
} | ||
|
||
/// <summary> | ||
/// Disables the Profiling integration. | ||
/// </summary> | ||
/// <param name="options">The SentryOptions to remove the integration from.</param> | ||
public static void DisableProfilingIntegration(this SentryOptions options) | ||
{ | ||
options.RemoveIntegration<ProfilingIntegration>(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
using Sentry.Extensibility; | ||
using Sentry.Integrations; | ||
|
||
namespace Sentry.Cocoa; | ||
|
||
/// <summary> | ||
/// Enables transaction performance profiling. | ||
/// </summary> | ||
public class ProfilingIntegration : ISdkIntegration | ||
{ | ||
/// <inheritdoc/> | ||
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."); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -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; | ||||||
|
||||||
/// <summary> | ||||||
/// Adds ProfilingIntegration to Sentry. | ||||||
/// </summary> | ||||||
/// <param name="startupTimeout"> | ||||||
/// Unused, only here so that the signature is the same as AddProfilingIntegration() from package Sentry.Profiling. | ||||||
/// </param> | ||||||
public void AddProfilingIntegration(TimeSpan startupTimeout = default) | ||||||
{ | ||||||
if (HasIntegration<ProfilingIntegration>()) | ||||||
{ | ||||||
if (_profilingIntegrationAddedByUser) | ||||||
{ | ||||||
DiagnosticLogger?.LogWarning($"{nameof(ProfilingIntegration)} has already been added. The second call to {nameof(AddProfilingIntegration)} will be ignored."); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondering if a warning is needed. Because if I have a shared Init logic for Sentry by used Apps, I just want profiling to be there. the fact iOS has it automatically but not the other platforms is irrelevant.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added the warning (actually the exact line of code) from the other profiling integration. Arguably, it may be unnecessary to warn when you add the same thing multiple times, no clue why integrations have warnings about that.
Doesn't play a role here. The warning only triggers when a user added the integration manually multiple times. Not when they add it once after we've added it automatically during init. I'll go ahead and merge this as is so it's exactly the same as the other profiling integration (which also warns only when you add it manually multiple times). We can change it later in all the places with he same LogWarning if you think we should |
||||||
} | ||||||
return; | ||||||
} | ||||||
|
||||||
_profilingIntegrationAddedByUser = true; | ||||||
AddIntegration(new ProfilingIntegration()); | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Disables the Profiling integration. | ||||||
/// </summary> | ||||||
public void DisableProfilingIntegration() | ||||||
{ | ||||||
_profilingIntegrationAddedByUser = false; | ||||||
RemoveIntegration<ProfilingIntegration>(); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<IBackgroundWorker>(), | ||
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<ISentryClient>()); | ||
|
||
private static IEnumerable<ISdkIntegration> GetIntegrations(ISentryClient hub) => | ||
hub.GetSentryOptions()?.Integrations ?? Enumerable.Empty<ISdkIntegration>(); | ||
|
||
[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); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could though
#if IOS
here and have a no-op version?"on iOS we use a native profiler"
Then it wouldn't matter during Set Up if we say
AddProfilerIntegration
on all platforms. It would just no-op on iOS?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about 5abd66d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! that looks good