Skip to content
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

ASPNET.Core default Environment settings are cleaned, if used. #554

Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# unreleased

* Set the Environment setting to 'production' if none was provided. (#550) @PureKrome
* ASPNET.Core hosting environment is set to 'production' / 'development' (notice lower casing) if no custom options.Enviroment is set. (#554) @PureKrome

# 3.0.0-alpha.1

Expand Down
50 changes: 46 additions & 4 deletions src/Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,52 @@ public override void Configure(SentryAspNetCoreOptions options)
{
base.Configure(options);

// Don't override user defined value
options.Environment ??=
EnvironmentLocator.Locate() // Sentry specific environment takes precedence #92
?? _hostingEnvironment.EnvironmentName;
// Don't override user defined value.
if (string.IsNullOrWhiteSpace(options.Environment))
{
var locatedEnvironment = EnvironmentLocator.Locate();
if (!string.IsNullOrWhiteSpace(locatedEnvironment))
{
// Sentry specific environment takes precedence #92.
options.Environment = locatedEnvironment;
}
else
{
// NOTE: Sentry prefers to have it's environment setting to be all lower case.
bruno-garcia marked this conversation as resolved.
Show resolved Hide resolved
// .NET Core sets the ENV variable to 'Production' (upper case P) or
// 'Development' (upper case D) which conflicts with the Sentry recommendation.
// As such, we'll be kind and override those values, here ... if applicable.
// Assumption: The Hosting Environment is always set.
// If not set by a developer, then the framework will auto set it.
// Alternatively, developers might set this to a CUSTOM value, which we
// need to respect (especially the case-sensitivity).
// REF: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/environments
#if NETSTANDARD2_0
if (_hostingEnvironment.EnvironmentName.Equals("Production"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider case sensitivity here? Or maybe use StringComparison.OrdinalIgnoreCase?

Copy link
Contributor Author

@PureKrome PureKrome Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: exact casing:

I did case sensitivity to reduce cycles. Why set the value if it's already the exact same?

EDIT: Also not sure if there's an extra allocation if setting the string value, if the previous string value is already the same.

{
options.Environment = Internal.Constants.DefaultEnvironmentSetting;
}
else if (_hostingEnvironment.EnvironmentName.Equals("Development"))
{
options.Environment = "development";
}
#else
if (_hostingEnvironment.EnvironmentName.Equals(Microsoft.Extensions.Hosting.Environments.Production))
PureKrome marked this conversation as resolved.
Show resolved Hide resolved
{
options.Environment = Internal.Constants.DefaultEnvironmentSetting;
}
else if (_hostingEnvironment.EnvironmentName.Equals(Microsoft.Extensions.Hosting.Environments.Development))
{
options.Environment = Microsoft.Extensions.Hosting.Environments.Development.ToLower();
PureKrome marked this conversation as resolved.
Show resolved Hide resolved
PureKrome marked this conversation as resolved.
Show resolved Hide resolved
}
#endif
PureKrome marked this conversation as resolved.
Show resolved Hide resolved
else
{
// Use the value set by the developer.
options.Environment = _hostingEnvironment.EnvironmentName;
}
}
}

options.AddLogEntryFilter((category, level, eventId, exception)
// https://github.com/aspnet/KestrelHttpServer/blob/0aff4a0440c2f393c0b98e9046a8e66e30a56cb0/src/Kestrel.Core/Internal/Infrastructure/KestrelTrace.cs#L33
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,5 +39,50 @@ public void Filters_KestrelEventId1_WithException_NotFiltered()
_sut.Configure(_target);
Assert.DoesNotContain(_target.Filters, f => f.Filter("Microsoft.AspNetCore.Server.Kestrel", LogLevel.Trace, 1, null));
}

[Theory]
[InlineData("foo", "Production", "foo")] // Custom - set. Rest ignored.
[InlineData("Production", "Production", "Production")] // Custom - set. Rest ignored. NOTE: Custom value _happens_ to be the common ASPNET_ENVIRONMENT PROD setting.
[InlineData(null, "Production", "production")] // Custom - nothing set. ASPNET_ENVIRONMENT is default PROD.
[InlineData("", "Production", "production")] // Custom - nothing set. ASPNET_ENVIRONMENT is default PROD.
[InlineData(null, "Development", "development")] // Custom - nothing set. ASPNET_ENVIRONMENT is default DEV.
[InlineData("", "Development", "development")] // Custom - nothing set. ASPNET_ENVIRONMENT is default DEV.
[InlineData(null, "production", "production")] // Custom - nothing set. ASPNET_ENVIRONMENT is custom (notice lowercase 'p').
[InlineData(null, "development", "development")] // Custom - nothing set. ASPNET_ENVIRONMENT is custom (notice lowercase 'd').
public void Filters_Environment_CustomOrASPNETEnvironment_Set(string environment, string hostingEnvironmentSetting, string expectedEnvironment)
{
// Arrange.
var hostingEnvironment = Substitute.For<IHostingEnvironment>();
hostingEnvironment.EnvironmentName = hostingEnvironmentSetting;

var sut = new SentryAspNetCoreOptionsSetup(
Substitute.For<ILoggerProviderConfiguration<SentryAspNetCoreLoggerProvider>>(),
hostingEnvironment);

//const string environment = "some environment";
_target.Environment = environment;

// Act.
sut.Configure(_target);

// Assert.
Assert.Equal(expectedEnvironment, _target.Environment);
}

[Theory]
[InlineData("foo")] // Random setting.
[InlineData("Production")] // Custom setting which is the same as ASPNET_ENVIRONMENT. But because this is manually set, don't change it.
public void Filters_Environment_SentryEnvironment_Set(string environment)
{
// Arrange.
EnvironmentVariableGuard.WithVariable(Internal.Constants.EnvironmentEnvironmentVariable, environment, () =>
PureKrome marked this conversation as resolved.
Show resolved Hide resolved
{
// Act.
_sut.Configure(_target);

// Assert.
Assert.Equal(environment, _target.Environment);
});
}
}
}