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

Request: please don't change case of environment #999

Closed
ajbeaven opened this issue May 16, 2021 · 4 comments · Fixed by #1057
Closed

Request: please don't change case of environment #999

ajbeaven opened this issue May 16, 2021 · 4 comments · Fixed by #1057
Labels
ASP.NET Core Feature New feature or request

Comments

@ajbeaven
Copy link
Contributor

ajbeaven commented May 16, 2021

In the .NET SDK, Sentry will change the case of the environment if the IHostingEnvironment.EnviromentName matches either "Production" or "Development". Any other environment names will not be changed.

// NOTE: Sentry prefers to have it's environment setting to be all lower case.
// .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 (_hostingEnvironment.EnvironmentName.Equals(Constants.ASPNETCoreProductionEnvironmentName))
{
options.Environment = Internal.Constants.ProductionEnvironmentSetting;
}
else if (_hostingEnvironment.EnvironmentName.Equals(Constants.ASPNETCoreDevelopmentEnvironmentName))
{
options.Environment = Internal.Constants.DevelopmentEnvironmentSetting;
}

I find this pretty strange, unexpected behaviour. I am migrating from .NET Framework to .NET Core and in so doing, want to stop explicitly specifying the environment and rely on the value specified in IHostingEnvironment. If I do this, due to this case change, I will then have the following environments listed in Sentry:

  • Production (now unused)
  • production
  • Development (now unused)
  • development
  • Test

I can hide the unused environments though this could cause problems viewing older issues. It's also a bit frustrating that the casing on my environments is no longer consistent (considering the Test environment). I admit these are minor inconveniences and I can just explicitly specify the environment to avoid this, but they are frustrations all the same. I can't think of a good reason why this undocumented behaviour exists and think generally it's best to display environments as they have been specified by the developer/framework.

The comment says that Sentry prefers lowercase environment names (though I haven't seen this echoed in the documentation). Is there any reason for this preference?

@Tyrrrz
Copy link
Contributor

Tyrrrz commented May 20, 2021

I think that might be because Sentry treats production and development as special values on ingestion, but I'm not sure.
@bruno-garcia should probably know.

@bruno-garcia
Copy link
Member

The change was added in the last major release and is documented in the migration docs:

https://docs.sentry.io/platforms/dotnet/migration/#environment-casing

That said, sorry for the inconvenience. This was discussed internally and since all Sentry SDKs send production, having only the .NET SDK sending Production resulted in 2 options on the selector if you had your frontend (any JS SDK for example) and the .NET SDK. Since the ASP.NET Core SDK was the only one sending a pascal case name we decided to align there.

We could add an opt-out for this behavior to the SentryAspNetCoreOptions so that the SDK doesn't mess with the names. Something like bool AdjustEnvironmentNameCasing. In hindsight we should have added this option from day 1 to allow users to easily opt-out of this rename. We'd be glad to merge a PR for this feature if you're keen to contribute.

@ajbeaven
Copy link
Contributor Author

Sure thing. I'm not precious about the PR, so feel free to throw it away if you reconsider the suggestion above.

I think probably a better option that would have avoided all of this would be to have the environment names be case insensitive when showing in the UI or selecting/specifying in the SDKs. Is it really a valid use to want to have separate Production and production environments?

In addition, you could lowercase the supplied Environment and use it as an ID, then add an option in the UI to specify whatever name someone wants to give it. That might be going a bit overboard.

@bruno-garcia
Copy link
Member

It's available on the latest preview: https://github.com/getsentry/sentry-dotnet/releases/tag/3.6.0-alpha.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASP.NET Core Feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants