feat: User.Id can now be overriden (set to null) in Global mode#5039
feat: User.Id can now be overriden (set to null) in Global mode#5039jamescrosswell wants to merge 8 commits intomainfrom
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Breaking Changes 🛠
Features ✨
Fixes 🐛
Dependencies ⬆️Deps
Other
🤖 This preview updates automatically when you update the PR. |
| } | ||
| eventLike.User.Id ??= _options.InstallationId; | ||
| // Set by the GlobalRootScopeIntegration In global mode so that it can be overridden by the user. | ||
| // In non-global mode (e.g. ASP.NET Core) the enricher sets it here as a fallback. |
There was a problem hiding this comment.
@Flash0ver arguably we could remove this code entirely... not sure if we actually want to set the same user id for all ASP.NET requests (where no user was logged in).
@bruno-garcia any thoughts from your side?
| ] | ||
| }, | ||
| { | ||
| Message: Starting BackpressureMonitor. |
There was a problem hiding this comment.
These were never really part of the test anyway (which is to ensure all the default integrations get registered)
There was a problem hiding this comment.
We could potentially set more stuff here. There are lots of scope properties that never change and which we currently set on the scope every time we capture an event. These are all candidates, I think:
sentry-dotnet/src/Sentry/Scope.cs
Lines 498 to 517 in f50b360
More of a performance improvement but might result in a subtle behavioural change for some users (perhaps code that checks/expects things to not be set in certain circumstances and sets them conditionally on this basis ). To be safe, perhaps we delay a change like that until the next major release.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5039 +/- ##
==========================================
- Coverage 74.03% 74.01% -0.02%
==========================================
Files 499 500 +1
Lines 18044 18057 +13
Branches 3510 3516 +6
==========================================
+ Hits 13358 13365 +7
- Misses 3830 3834 +4
- Partials 856 858 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adjusts how User.Id is populated from InstallationId when Global Mode is enabled, so that applications can explicitly clear/override the value (e.g., set it to null) without it being re-applied during event enrichment.
Changes:
- Add a new
GlobalRootScopeIntegrationdefault integration that setsscope.User.Idfromoptions.InstallationIdat startup (Global Mode only). - Update
Enricherto not apply theInstallationIdfallback in Global Mode (so user overrides aren’t re-written). - Update/extend tests and Verify snapshots to account for the new default integration and changed behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/Sentry/Integrations/GlobalRootScopeIntegration.cs |
New integration to set User.Id on the root scope in Global Mode. |
src/Sentry/Internal/Enricher.cs |
Stops applying InstallationId fallback when Global Mode is enabled. |
src/Sentry/SentryOptions.cs |
Registers the new integration as a default integration. |
test/Sentry.Tests/Integrations/GlobalRootScopeIntegrationTests.cs |
Adds tests covering the new integration and updated enricher behavior. |
test/Sentry.Tests/SentryClientTests.cs |
Adjusts tests around fallback User.Id behavior. |
test/Sentry.Tests/SentryOptionsTests.verify.cs + *.verified.txt |
Narrows snapshot scope to integration-registration logs and updates expected output. |
src/Sentry/PlatformAbstractions/FrameworkInfo.cs |
Adds an additional .NET Framework release key mapping for 4.8.1. |
| #if NET8_0_OR_GREATER | ||
| | DefaultIntegrations.SystemDiagnosticsMetricsIntegration | ||
| #endif | ||
| | DefaultIntegrations.GlobalRootScopeIntegration | ||
| ; |
There was a problem hiding this comment.
GlobalRootScopeIntegration is now enabled by default, but unlike the other default integrations there’s no public Disable* method to let consumers turn it off. Consider adding a DisableGlobalRootScopeIntegration() (or similar) for parity and to give users a supported opt-out.
There was a problem hiding this comment.
Arguably it's not an 'integration'... it's just some behaviour we want on all global root scopes (there's no need for an opt-out). We could move it out of the integrations list and bake the logic into the Hub constructor instead.
Otherwise I think it's OK to simply omit the disable integration method for this one. @Flash0ver thoughts?
| integration.Register(hub, options); | ||
|
|
||
| // Assert | ||
| hub.Received(1).ConfigureScope(Arg.Any<Action<Scope>>()); |
There was a problem hiding this comment.
Test name says it “SetsInstallationIdOnRootScope”, but the assertion only verifies that ConfigureScope is called. Either update the assertion to validate the configured action sets User.Id from InstallationId (similar to the later test), or rename this test to reflect what it actually checks.
| hub.Received(1).ConfigureScope(Arg.Any<Action<Scope>>()); | |
| hub.Received(1).ConfigureScope(Arg.Do<Action<Scope>>(configure => | |
| { | |
| var scope = new Scope(options); | |
| configure(scope); | |
| Assert.Equal(options.InstallationId, scope.User?.Id); | |
| })); |
src/Sentry/Internal/Enricher.cs
Outdated
| // Set by the GlobalRootScopeIntegration In global mode so that it can be overridden by the user. | ||
| // In non-global mode (e.g. ASP.NET Core) the enricher sets it here as a fallback. | ||
| if (!_options.IsGlobalModeEnabled) | ||
| { | ||
| eventLike.User.Id ??= _options.InstallationId; | ||
| } | ||
|
|
There was a problem hiding this comment.
With this change, the InstallationId fallback for User.Id is no longer applied when IsGlobalModeEnabled is true. That means scenarios that rely on Enricher without going through Hub initialization/integrations (e.g., apps constructing SentryClient directly) will no longer get the previous default User.Id behavior in global mode. If that behavior needs to remain, consider an alternative that still allows explicit clearing (e.g., apply the fallback when creating the initial Scope/root scope, or introduce an explicit option/flag indicating whether the fallback should be applied).
| // Set by the GlobalRootScopeIntegration In global mode so that it can be overridden by the user. | |
| // In non-global mode (e.g. ASP.NET Core) the enricher sets it here as a fallback. | |
| if (!_options.IsGlobalModeEnabled) | |
| { | |
| eventLike.User.Id ??= _options.InstallationId; | |
| } | |
| // User.Id can be set by the GlobalRootScopeIntegration in global mode or by user code. | |
| // The enricher still provides the InstallationId-based fallback here, but it will not override | |
| // any value that was already set. | |
| eventLike.User.Id ??= _options.InstallationId; |
There was a problem hiding this comment.
I don't think there's a scenario where someone uses a SentryClient independent of the Hub... so this isn't a real concern.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Resolves #4172