From 7545170f3cc44b6e6314d6a2c35847c570e55051 Mon Sep 17 00:00:00 2001 From: Pure Krome Date: Thu, 15 Oct 2020 13:43:16 +1100 Subject: [PATCH 1/7] Default environment setting if none provided. Fixes #549. --- src/Sentry/SentryClient.cs | 9 ++++ .../IntegrationMockedBackgroundWorker.cs | 1 + test/Sentry.Tests/SentryClientTests.cs | 41 +++++++++++++++++++ 3 files changed, 51 insertions(+) diff --git a/src/Sentry/SentryClient.cs b/src/Sentry/SentryClient.cs index 553c1da956..7c99da9783 100644 --- a/src/Sentry/SentryClient.cs +++ b/src/Sentry/SentryClient.cs @@ -146,6 +146,15 @@ private SentryId DoSendEvent(SentryEvent @event, Scope? scope) } } + // Each event should have an 'Environment' set. If none was set, then should default + // to 'production'. + if (string.IsNullOrWhiteSpace(@event.Environment)) + { + @event.Environment = string.IsNullOrWhiteSpace(_options.Environment) + ? "production" + : _options.Environment; + } + SentryEvent? processedEvent = @event; foreach (var processor in scope.GetAllEventProcessors()) diff --git a/test/Sentry.AspNetCore.Tests/IntegrationMockedBackgroundWorker.cs b/test/Sentry.AspNetCore.Tests/IntegrationMockedBackgroundWorker.cs index 0889e60f86..887ec07509 100644 --- a/test/Sentry.AspNetCore.Tests/IntegrationMockedBackgroundWorker.cs +++ b/test/Sentry.AspNetCore.Tests/IntegrationMockedBackgroundWorker.cs @@ -234,6 +234,7 @@ public void Environment_NotOnOptions_ValueFromEnvVar() }); } + // NOTE: Options take priority over EnvVar's. [Fact] public void Environment_BothOnOptionsAndEnvVar_ValueFromOption() { diff --git a/test/Sentry.Tests/SentryClientTests.cs b/test/Sentry.Tests/SentryClientTests.cs index 804912cd0a..7f03d8fe1c 100644 --- a/test/Sentry.Tests/SentryClientTests.cs +++ b/test/Sentry.Tests/SentryClientTests.cs @@ -289,6 +289,47 @@ public void CaptureEvent_DisposedClient_ThrowsObjectDisposedException() _ = Assert.Throws(() => sut.CaptureEvent(null)); } + [Theory] + [InlineData(null, "production")] // Missing: will get default value. + [InlineData("", "production")] // Missing: will get default value. + [InlineData(" ", "production")] // Missing: will get default value. + [InlineData("a", "a")] // Provided: nothing will change. + [InlineData("production", "production")] // Provided: nothing will change. (value happens to be the default) + [InlineData("aBcDe F !@#$ gHi", "aBcDe F !@#$ gHi")] // Provided: nothing will change. (Case check) + public void CaptureEvent_Environment_HasCorrectEnvironmentValue(string environment, string expectedEnvironment) + { + //_fixture.SentryOptions.Environment = environment; + + var @event = new SentryEvent + { + Environment = environment + }; + + var sut = _fixture.GetSut(); + _ = sut.CaptureEvent(@event); + + Assert.Equal(expectedEnvironment, @event.Environment); + } + + [Theory] + [InlineData(null, "production")] // Missing: will get default value. + [InlineData("", "production")] // Missing: will get default value. + [InlineData(" ", "production")] // Missing: will get default value. + [InlineData("a", "a")] // Provided: nothing will change. + [InlineData("production", "production")] // Provided: nothing will change. (value happens to be the default) + [InlineData("aBcDe F !@#$ gHi", "aBcDe F !@#$ gHi")] // Provided: nothing will change. (Case check) + public void CaptureEvent_OptionsEnvironment_HasCorrectEnvironmentValue(string environment, string expectedEnvironment) + { + _fixture.SentryOptions.Environment = environment; + + var @event = new SentryEvent(); + + var sut = _fixture.GetSut(); + _ = sut.CaptureEvent(@event); + + Assert.Equal(expectedEnvironment, @event.Environment); + } + [Fact] public void Dispose_Worker_DisposeCalled() { From f4ad0f9dfc31e3266ba9183123bb80c09ac0f915 Mon Sep 17 00:00:00 2001 From: Pure Krome Date: Thu, 15 Oct 2020 14:00:00 +1100 Subject: [PATCH 2/7] :lipstick: Include #550 PR info. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2ddf1276a..3b8dc31cfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ * Rename `LogEntry` to `SentryMessage`. Change type of `SentryEvent.Message` from `string` to `SentryMessage`. * Change the type of `Gpu.VendorId` from `int` to `string`. +* Set the Environment setting to 'production' if none was provided. (#550) @PureKrome # 3.0.0-alpha.0 From 3752c438ba960170c36ae57aa7c8f4809b4c649a Mon Sep 17 00:00:00 2001 From: Pure Krome Date: Thu, 15 Oct 2020 14:43:51 +1100 Subject: [PATCH 3/7] :wrench: Moved Environment check over to MainSentryEventProcessor. --- src/Sentry/Internal/Constants.cs | 6 +++ .../Internal/MainSentryEventProcessor.cs | 14 ++++++- src/Sentry/SentryClient.cs | 9 ---- .../MainSentryEventProcessorTests.cs | 30 +++++++++----- test/Sentry.Tests/SentryClientTests.cs | 41 ------------------- 5 files changed, 38 insertions(+), 62 deletions(-) diff --git a/src/Sentry/Internal/Constants.cs b/src/Sentry/Internal/Constants.cs index b183464d3b..ab7ddfba77 100644 --- a/src/Sentry/Internal/Constants.cs +++ b/src/Sentry/Internal/Constants.cs @@ -20,6 +20,12 @@ internal static class Constants /// public const string EnvironmentEnvironmentVariable = "SENTRY_ENVIRONMENT"; + /// + /// Default Sentry environment setting. + /// + /// Best Sentry practice is to always try and have a value for this setting. + public const string DefaultEnvironmentSetting = "Production"; + // See: https://github.com/getsentry/sentry-release-registry public const string SdkName = "sentry.dotnet"; } diff --git a/src/Sentry/Internal/MainSentryEventProcessor.cs b/src/Sentry/Internal/MainSentryEventProcessor.cs index 170557953f..4699ce7a14 100644 --- a/src/Sentry/Internal/MainSentryEventProcessor.cs +++ b/src/Sentry/Internal/MainSentryEventProcessor.cs @@ -136,9 +136,19 @@ public SentryEvent Process(SentryEvent @event) @event.Release = _options.Release ?? Release; } - if (@event.Environment == null) + if (string.IsNullOrWhiteSpace(@event.Environment)) { - @event.Environment = _options.Environment ?? EnvironmentLocator.Locate(); + if (string.IsNullOrWhiteSpace(_options.Environment)) + { + var foundEnvironment = EnvironmentLocator.Locate(); + @event.Environment = string.IsNullOrWhiteSpace(foundEnvironment) + ? Constants.DefaultEnvironmentSetting + : foundEnvironment; + } + else + { + @event.Environment = _options.Environment; + } } if (@event.Exception == null) diff --git a/src/Sentry/SentryClient.cs b/src/Sentry/SentryClient.cs index 7c99da9783..553c1da956 100644 --- a/src/Sentry/SentryClient.cs +++ b/src/Sentry/SentryClient.cs @@ -146,15 +146,6 @@ private SentryId DoSendEvent(SentryEvent @event, Scope? scope) } } - // Each event should have an 'Environment' set. If none was set, then should default - // to 'production'. - if (string.IsNullOrWhiteSpace(@event.Environment)) - { - @event.Environment = string.IsNullOrWhiteSpace(_options.Environment) - ? "production" - : _options.Environment; - } - SentryEvent? processedEvent = @event; foreach (var processor in scope.GetAllEventProcessors()) diff --git a/test/Sentry.Tests/Internals/MainSentryEventProcessorTests.cs b/test/Sentry.Tests/Internals/MainSentryEventProcessorTests.cs index 5ae28b7d24..5f0bea683c 100644 --- a/test/Sentry.Tests/Internals/MainSentryEventProcessorTests.cs +++ b/test/Sentry.Tests/Internals/MainSentryEventProcessorTests.cs @@ -164,35 +164,45 @@ public void Process_NoReleaseOnOptions_SameAsCachedVersion() Assert.Equal(sut.Release, evt.Release); } - [Fact] - public void Process_EnvironmentOnOptions_SetToEvent() + [Theory] + [InlineData(null, Constants.DefaultEnvironmentSetting)] // Missing: will get default value. + [InlineData("", Constants.DefaultEnvironmentSetting)] // Missing: will get default value. + [InlineData(" ", Constants.DefaultEnvironmentSetting)] // Missing: will get default value. + [InlineData("a", "a")] // Provided: nothing will change. + [InlineData("production", "production")] // Provided: nothing will change. (value has a lower case 'p', different to default value) + [InlineData("aBcDe F !@#$ gHi", "aBcDe F !@#$ gHi")] // Provided: nothing will change. (Case check) + public void Process_EnvironmentOnOptions_SetToEvent(string environment, string expectedEnvironment) { - const string expected = "Production"; - _fixture.SentryOptions.Environment = expected; + _fixture.SentryOptions.Environment = environment; var sut = _fixture.GetSut(); var evt = new SentryEvent(); _ = sut.Process(evt); - Assert.Equal(expected, evt.Environment); + Assert.Equal(expectedEnvironment, evt.Environment); } - [Fact] - public void Process_NoEnvironmentOnOptions_SameAsEnvironmentVariable() + [Theory] + [InlineData(null, Constants.DefaultEnvironmentSetting)] // Missing: will get default value. + [InlineData("", Constants.DefaultEnvironmentSetting)] // Missing: will get default value. + [InlineData(" ", Constants.DefaultEnvironmentSetting)] // Missing: will get default value. + [InlineData("a", "a")] // Provided: nothing will change. + [InlineData("production", "production")] // Provided: nothing will change. (value has a lower case 'p', different to default value) + [InlineData("aBcDe F !@#$ gHi", "aBcDe F !@#$ gHi")] // Provided: nothing will change. (Case check) + public void Process_NoEnvironmentOnOptions_SameAsEnvironmentVariable(string environment, string expectedEnvironment) { - const string expected = "Staging"; var sut = _fixture.GetSut(); var evt = new SentryEvent(); EnvironmentVariableGuard.WithVariable( Constants.EnvironmentEnvironmentVariable, - expected, + environment, () => { _ = sut.Process(evt); }); - Assert.Equal(expected, evt.Environment); + Assert.Equal(expectedEnvironment, evt.Environment); } [Fact] diff --git a/test/Sentry.Tests/SentryClientTests.cs b/test/Sentry.Tests/SentryClientTests.cs index 7f03d8fe1c..804912cd0a 100644 --- a/test/Sentry.Tests/SentryClientTests.cs +++ b/test/Sentry.Tests/SentryClientTests.cs @@ -289,47 +289,6 @@ public void CaptureEvent_DisposedClient_ThrowsObjectDisposedException() _ = Assert.Throws(() => sut.CaptureEvent(null)); } - [Theory] - [InlineData(null, "production")] // Missing: will get default value. - [InlineData("", "production")] // Missing: will get default value. - [InlineData(" ", "production")] // Missing: will get default value. - [InlineData("a", "a")] // Provided: nothing will change. - [InlineData("production", "production")] // Provided: nothing will change. (value happens to be the default) - [InlineData("aBcDe F !@#$ gHi", "aBcDe F !@#$ gHi")] // Provided: nothing will change. (Case check) - public void CaptureEvent_Environment_HasCorrectEnvironmentValue(string environment, string expectedEnvironment) - { - //_fixture.SentryOptions.Environment = environment; - - var @event = new SentryEvent - { - Environment = environment - }; - - var sut = _fixture.GetSut(); - _ = sut.CaptureEvent(@event); - - Assert.Equal(expectedEnvironment, @event.Environment); - } - - [Theory] - [InlineData(null, "production")] // Missing: will get default value. - [InlineData("", "production")] // Missing: will get default value. - [InlineData(" ", "production")] // Missing: will get default value. - [InlineData("a", "a")] // Provided: nothing will change. - [InlineData("production", "production")] // Provided: nothing will change. (value happens to be the default) - [InlineData("aBcDe F !@#$ gHi", "aBcDe F !@#$ gHi")] // Provided: nothing will change. (Case check) - public void CaptureEvent_OptionsEnvironment_HasCorrectEnvironmentValue(string environment, string expectedEnvironment) - { - _fixture.SentryOptions.Environment = environment; - - var @event = new SentryEvent(); - - var sut = _fixture.GetSut(); - _ = sut.CaptureEvent(@event); - - Assert.Equal(expectedEnvironment, @event.Environment); - } - [Fact] public void Dispose_Worker_DisposeCalled() { From cc4f725b622aa232f7384db01ab12dd87f90be50 Mon Sep 17 00:00:00 2001 From: Pure Krome Date: Fri, 16 Oct 2020 08:45:57 +1100 Subject: [PATCH 4/7] Update src/Sentry/Internal/Constants.cs Co-authored-by: Bruno Garcia --- src/Sentry/Internal/Constants.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Sentry/Internal/Constants.cs b/src/Sentry/Internal/Constants.cs index ab7ddfba77..cc3296abb8 100644 --- a/src/Sentry/Internal/Constants.cs +++ b/src/Sentry/Internal/Constants.cs @@ -24,7 +24,7 @@ internal static class Constants /// Default Sentry environment setting. /// /// Best Sentry practice is to always try and have a value for this setting. - public const string DefaultEnvironmentSetting = "Production"; + public const string DefaultEnvironmentSetting = "production"; // See: https://github.com/getsentry/sentry-release-registry public const string SdkName = "sentry.dotnet"; From f29d70be8befa7b7a5c4b74c138432efc0ac8934 Mon Sep 17 00:00:00 2001 From: Pure Krome Date: Fri, 16 Oct 2020 13:04:44 +1100 Subject: [PATCH 5/7] Update test/Sentry.Tests/Internals/MainSentryEventProcessorTests.cs Co-authored-by: Bruno Garcia --- test/Sentry.Tests/Internals/MainSentryEventProcessorTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Sentry.Tests/Internals/MainSentryEventProcessorTests.cs b/test/Sentry.Tests/Internals/MainSentryEventProcessorTests.cs index 5f0bea683c..b24faf14f7 100644 --- a/test/Sentry.Tests/Internals/MainSentryEventProcessorTests.cs +++ b/test/Sentry.Tests/Internals/MainSentryEventProcessorTests.cs @@ -187,7 +187,7 @@ public void Process_EnvironmentOnOptions_SetToEvent(string environment, string e [InlineData("", Constants.DefaultEnvironmentSetting)] // Missing: will get default value. [InlineData(" ", Constants.DefaultEnvironmentSetting)] // Missing: will get default value. [InlineData("a", "a")] // Provided: nothing will change. - [InlineData("production", "production")] // Provided: nothing will change. (value has a lower case 'p', different to default value) + [InlineData("Production", "Production")] // Provided: nothing will change. (value has a upper case 'p', different to default value) [InlineData("aBcDe F !@#$ gHi", "aBcDe F !@#$ gHi")] // Provided: nothing will change. (Case check) public void Process_NoEnvironmentOnOptions_SameAsEnvironmentVariable(string environment, string expectedEnvironment) { From d517f0db51e93840741d6b41db0407864caf2d76 Mon Sep 17 00:00:00 2001 From: Pure Krome Date: Fri, 16 Oct 2020 17:12:34 +1100 Subject: [PATCH 6/7] :lipstick: PR feedback. --- .../Sentry.AspNetCore.Tests/IntegrationMockedBackgroundWorker.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/test/Sentry.AspNetCore.Tests/IntegrationMockedBackgroundWorker.cs b/test/Sentry.AspNetCore.Tests/IntegrationMockedBackgroundWorker.cs index 887ec07509..0889e60f86 100644 --- a/test/Sentry.AspNetCore.Tests/IntegrationMockedBackgroundWorker.cs +++ b/test/Sentry.AspNetCore.Tests/IntegrationMockedBackgroundWorker.cs @@ -234,7 +234,6 @@ public void Environment_NotOnOptions_ValueFromEnvVar() }); } - // NOTE: Options take priority over EnvVar's. [Fact] public void Environment_BothOnOptionsAndEnvVar_ValueFromOption() { From 687acd2465999d983370ccac5247a68f7ccb2fd8 Mon Sep 17 00:00:00 2001 From: Pure Krome Date: Sat, 17 Oct 2020 00:04:31 +1100 Subject: [PATCH 7/7] Env Var takes priority over options. --- src/Sentry/Internal/MainSentryEventProcessor.cs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/Sentry/Internal/MainSentryEventProcessor.cs b/src/Sentry/Internal/MainSentryEventProcessor.cs index 4699ce7a14..536e8fb9db 100644 --- a/src/Sentry/Internal/MainSentryEventProcessor.cs +++ b/src/Sentry/Internal/MainSentryEventProcessor.cs @@ -136,19 +136,16 @@ public SentryEvent Process(SentryEvent @event) @event.Release = _options.Release ?? Release; } + // Recommendation: The 'Environment' setting should always be set + // with a default fallback. if (string.IsNullOrWhiteSpace(@event.Environment)) { - if (string.IsNullOrWhiteSpace(_options.Environment)) - { - var foundEnvironment = EnvironmentLocator.Locate(); - @event.Environment = string.IsNullOrWhiteSpace(foundEnvironment) + var foundEnvironment = EnvironmentLocator.Locate(); + @event.Environment = string.IsNullOrWhiteSpace(foundEnvironment) + ? string.IsNullOrWhiteSpace(_options.Environment) ? Constants.DefaultEnvironmentSetting - : foundEnvironment; - } - else - { - @event.Environment = _options.Environment; - } + : _options.Environment + : foundEnvironment; } if (@event.Exception == null)