-
-
Notifications
You must be signed in to change notification settings - Fork 231
feat: User.Id can now be overriden (set to null) in Global mode #5039
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
base: main
Are you sure you want to change the base?
Changes from all commits
28e9845
1b15d0f
7b2e38d
7a7d0f6
9907a6c
2663ae0
abca943
8c2d66b
04435e6
219f31e
8ae30ad
4d523b3
070e294
3141233
af186b3
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,14 @@ | ||
| namespace Sentry.Integrations; | ||
|
|
||
| internal class GlobalRootScopeIntegration : ISdkIntegration | ||
| { | ||
| public void Register(IHub hub, SentryOptions options) | ||
| { | ||
| if (!options.IsGlobalModeEnabled) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| hub.ConfigureScope(scope => scope.User.Id ??= options.InstallationId); | ||
|
Member
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. question: compliant with new PII rules? See https://develop.sentry.dev/sdk/foundations/client/data-collection/. I just wanted to make sure we don't paint ourselves into a corner when - later this year - we implement the new PII rules via a new
Collaborator
Author
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. Without the installation id, some functionality like Session Health simply doesn't work. So I can't see how we can avoid setting this. This PR also doesn't introduce the collection of any new information - it just shifts when we're collecting it. So if we no longer want to generate installation ids and send these with events etc. that's fine - but I don't think related to this PR (that would be a separate issue and separate PR in the backlog). Arguably this PR improves the situation since previously users couldn't opt out of the installation id being set/collected. Now, since we set it early, they can override it by nulling it out if they want. |
||
| } | ||
|
jamescrosswell marked this conversation as resolved.
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| using Sentry.Integrations; | ||
|
|
||
| namespace Sentry.Tests.Integrations; | ||
|
|
||
| public class GlobalRootScopeIntegrationTests | ||
| { | ||
| [Fact] | ||
| public void Register_GlobalModeDisabled_DoesNotConfigureScope() | ||
| { | ||
| // Arrange | ||
| var options = new SentryOptions | ||
| { | ||
| Dsn = ValidDsn, | ||
| IsGlobalModeEnabled = false, | ||
| AutoSessionTracking = false | ||
| }; | ||
| var scope = new Scope(); | ||
|
|
||
| var hub = Substitute.For<IHub>(); | ||
| hub.SubstituteConfigureScope(scope); | ||
| var integration = new GlobalRootScopeIntegration(); | ||
|
|
||
| // Act | ||
| integration.Register(hub, options); | ||
|
|
||
| // Assert | ||
| hub.DidNotReceive().ConfigureScope(Arg.Any<Action<Scope>>()); | ||
|
jamescrosswell marked this conversation as resolved.
|
||
| scope.User.Id.Should().BeNull(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Register_GlobalModeEnabled_SetsInstallationIdOnRootScope() | ||
| { | ||
| // Arrange | ||
| var options = new SentryOptions | ||
| { | ||
| Dsn = ValidDsn, | ||
| IsGlobalModeEnabled = true, | ||
| AutoSessionTracking = false | ||
| }; | ||
| var scope = new Scope(); | ||
|
|
||
| var hub = Substitute.For<IHub>(); | ||
| hub.SubstituteConfigureScope(scope); | ||
| var integration = new GlobalRootScopeIntegration(); | ||
|
|
||
| // Act | ||
| integration.Register(hub, options); | ||
|
|
||
| // Assert | ||
| hub.Received(1).ConfigureScope(Arg.Any<Action<Scope>>()); | ||
| scope.User.Id.Should().Be(options.InstallationId); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Register_GlobalModeEnabled_DoesNotOverwriteExistingUserId() | ||
| { | ||
| // Arrange | ||
| var options = new SentryOptions | ||
| { | ||
| Dsn = ValidDsn, | ||
| IsGlobalModeEnabled = true, | ||
| AutoSessionTracking = false | ||
| }; | ||
| var oldId = "old-id"; | ||
| var scope = new Scope | ||
| { | ||
| User = | ||
| { | ||
| Id = oldId | ||
| } | ||
| }; | ||
|
|
||
| var hub = Substitute.For<IHub>(); | ||
| hub.SubstituteConfigureScope(scope); | ||
| var integration = new GlobalRootScopeIntegration(); | ||
|
|
||
| // Act | ||
| integration.Register(hub, options); | ||
|
|
||
| // Assert | ||
| hub.Received(1).ConfigureScope(Arg.Any<Action<Scope>>()); | ||
| scope.User.Id.Should().Be(oldId); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Enricher_GlobalModeEnabled_DoesNotSetInstallationId() | ||
| { | ||
| // Verify the enricher no longer sets User.Id when global mode is enabled, | ||
| // ensuring users can clear the User.Id set by GlobalRootScopeIntegration. | ||
| var options = new SentryOptions { IsGlobalModeEnabled = true }; | ||
| var enricher = new Sentry.Internal.Enricher(options); | ||
|
|
||
| var eventLike = Substitute.For<IEventLike>(); | ||
| eventLike.Sdk.Returns(new SdkVersion()); | ||
| eventLike.User = new SentryUser(); | ||
| eventLike.Contexts = new SentryContexts(); | ||
|
|
||
| enricher.Apply(eventLike); | ||
|
|
||
| eventLike.User.Id.Should().BeNull(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Enricher_GlobalModeDisabled_SetsInstallationIdAsFallback() | ||
| { | ||
| // Verify the enricher still sets User.Id when global mode is disabled (e.g. ASP.NET Core). | ||
| var options = new SentryOptions { IsGlobalModeEnabled = false }; | ||
| var enricher = new Sentry.Internal.Enricher(options); | ||
|
|
||
| var eventLike = Substitute.For<IEventLike>(); | ||
| eventLike.Sdk.Returns(new SdkVersion()); | ||
| eventLike.User = new SentryUser(); | ||
| eventLike.Contexts = new SentryContexts(); | ||
|
|
||
| enricher.Apply(eventLike); | ||
|
|
||
| eventLike.User.Id.Should().Be(options.InstallationId); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,4 @@ | ||
| [ | ||
| { | ||
| Message: Initializing Hub for Dsn: '{0}'., | ||
| Args: [ | ||
| https://d4d82fc1c2c4032a83f3a29aa3a3aff@fake-sentry.io:65535/2147483647 | ||
| ] | ||
| }, | ||
| { | ||
| Message: Starting BackpressureMonitor. | ||
|
Collaborator
Author
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. These were never really part of the test anyway (which is to ensure all the default integrations get registered) |
||
| }, | ||
| { | ||
| Message: Registering integration: '{0}'., | ||
| Args: [ | ||
|
|
@@ -37,5 +28,11 @@ | |
| Args: [ | ||
| SentryDiagnosticListenerIntegration | ||
| ] | ||
| }, | ||
| { | ||
| Message: Registering integration: '{0}'., | ||
| Args: [ | ||
| GlobalRootScopeIntegration | ||
| ] | ||
| } | ||
| ] | ||
Uh oh!
There was an error while loading. Please reload this page.