-
-
Notifications
You must be signed in to change notification settings - Fork 238
feat: sync environment to the native layers
#5365
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
cc9de07
925a20a
f8f932a
6a90a32
d2b5eb5
c6268d0
b4821a3
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 | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -35,6 +35,11 @@ public interface IScopeObserver | |||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||
| public void SetTrace(SentryId traceId, SpanId parentSpanId); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||
| /// Sets the environment. | ||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||
| public void SetEnvironment(string? environment); | ||||||||||||||||||||||
|
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. thought: Making the ScopeObserver an I wonder if an Changing this
Collaborator
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. We could make the change in the next major version... I think it's a good suggestion. We can mock/substitute abstract classes just as well as interfaces.
Contributor
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. I understand the "technically source breaking" argument but we have this discussion every time I make changes to the interface. The thing should go being For what it's worth we're not really breaking user code. This is a hypothetical. The
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. Can / should we make sentry-dotnet/src/Sentry/SentryOptions.cs Lines 86 to 95 in ebef6ff
Users might today already provide their own Scope-Observer. The So we do have a bit of an incomplete story here.
Contributor
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. The wrapping is happening because you guys asked me to respect any potentially added ScopeObserver since it is public. Fair enough. But that should not be a precedence of us having to support this or not change it. We have a very limited set of hooks that we want to support on the SDK, right? Where users are supposed to filter, mutate or drop data/events, i.e. in We can continue weighing the pros and cons of dropping it in #4529
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. sounds good ... I actually didn't quite intend to spawn off a heavy discussion on this ... just wanted to note, that for the next |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||
| /// Adds an attachment. | ||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -144,8 +144,35 @@ public SentryUser User | |
| /// <inheritdoc /> | ||
| public string? Distribution { get; set; } | ||
|
|
||
| private string? _environment; | ||
|
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. nitpick: could use If we don't use the See https://learn.microsoft.com/dotnet/csharp/whats-new/csharp-14#the-field-keyword |
||
|
|
||
| /// <inheritdoc /> | ||
| public string? Environment { get; set; } | ||
| public string? Environment | ||
| { | ||
| get => _environment; | ||
| set | ||
| { | ||
| if (_environment == value) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (value is null) | ||
| { | ||
| Options.LogWarning("Environment cannot be null. Reverting to default value from the options."); | ||
| _environment = Options.Environment; | ||
| } | ||
| else | ||
| { | ||
| _environment = value; | ||
| } | ||
|
|
||
| if (Options is { EnableScopeSync: true, ScopeObserver: { } observer }) | ||
| { | ||
| observer.SetEnvironment(_environment); | ||
| } | ||
| } | ||
| } | ||
|
bitsandfoxes marked this conversation as resolved.
|
||
|
|
||
| // TransactionName is kept for legacy purposes because | ||
| // SentryEvent still makes use of it. | ||
|
|
@@ -288,6 +315,8 @@ internal Scope(SentryOptions? options, SentryPropagationContext? propagationCont | |
| { | ||
| Options = options ?? new SentryOptions(); | ||
| PropagationContext = new SentryPropagationContext(propagationContext); | ||
|
|
||
| _environment = Options.Environment; | ||
| } | ||
|
|
||
| // For testing. Should explicitly require SentryOptions. | ||
|
|
@@ -406,7 +435,7 @@ public void Clear() | |
| User = new(); | ||
| Release = default; | ||
| Distribution = default; | ||
| Environment = default; | ||
| Environment = Options.Environment; // Restore to keep in sync with native | ||
| TransactionName = default; | ||
| Transaction = default; | ||
| Fingerprint = Array.Empty<string>(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.