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

Include staging in the environment naming rules from HostEnvironment #1046

Merged
merged 4 commits into from
Jun 10, 2021

Conversation

Turnerj
Copy link
Contributor

@Turnerj Turnerj commented Jun 6, 2021

Continues on from where #551 / #554 left off with the environment naming. Like "Production" and "Development", the environment "Staging" is built into the framework. For consistency, I think it is important that all 3 of these built-in names perform the same way.

Additionally, I've switched to use the extension methods to check the environment names. Logically they would perform the same action but it is the "standard" way of checking the environments.

@Turnerj
Copy link
Contributor Author

Turnerj commented Jun 6, 2021

While I'm obviously proposing this, I'm actually not entirely happy with the inconsistency with the environment name handling. To be honest, I don't think there should be any manipulation of the hosting environment name even if it is to match the other Sentry SDKs.

As a .NET developer, you might know specifically the environment names are "Production", "Development" etc but that alone wouldn't seem weird in Sentry as "production" and "development" - it just looks like you've called ToLower on it. It gets weird though when I have a custom environment name in HostEnvironment (eg. "Test" - or in the case that lead to this PR, the non-custom name "Staging") which then is sent through to Sentry as-is. I get that Sentry is case-sensitive with environment names, just from someone dealing with HostEnvironment, it seems like a hidden/obscure behaviour.

I'd be tempted to just have _hostingEnvironment.EnvironmentName.ToLower() instead if we just wanted lowercase names. I get that would break a lot of people's workflows (and would be inconsistent with the other language SDKs) but it would at least seem consistent from the scope of using Sentry with .NET.

@bruno-garcia
Copy link
Member

bruno-garcia commented Jun 7, 2021

I'm happy to merge this change though it's a rather breaking change (not in API but in the data in Sentry though) and ideally we'd only ship this in the next major.

While I do agree it sucks to have this ToLower here, it was agreed in Sentry to have lower case environment names. Other SDKs will report those in lower case and if you have JS+.NET it'll end up having two environments by default.

Using the .NET SDK outside ASP.NET Core reports production, so if we didn't lower the ASP.NET Core default 'Production' we'd also have two environments.

Reverting this is probably not something we want to do. @mitsuhiko could comment on the subject as he had strong opinions about having this lowercase.

@bruno-garcia bruno-garcia mentioned this pull request Jun 7, 2021
@bruno-garcia
Copy link
Member

bruno-garcia commented Jun 7, 2021

Could you please add a changelog entry:

### Fixes

- Report lowercase staging environment for ASP.NET Core ([#1046](https://github.com/getsentry/sentry-unity/pull/1046))

@Turnerj
Copy link
Contributor Author

Turnerj commented Jun 8, 2021

Could you please add a changelog entry:

Done.

I'm happy to merge this change though it's a rather breaking change (not in API but in the data in Sentry though) and ideally we'd only ship this in the next major.

Yeah, I definitely can understand that. I didn't actually notice in the v3 change logs that it did change the case so I was wondering what I did wrong as to why there were two different environment names in my instance - particularly that development and production changed but staging didn't.

It was only when I was going to file an issue about it did I find the reference in the issues & change log - then I decided to open this PR instead.

Using the .NET SDK outside ASP.NET Core reports production, so if we didn't lower the ASP.NET Core default 'Production' we'd also have two environments.

Yeah, that would also suck. What I'm thinking is actually going further for consistency by replacing this entire section:

https://github.com/getsentry/sentry-dotnet/pull/1046/files#diff-07aa862a2e3e6d78662cc98020e4a5b12605159a48c88961df0377a57eb503f7R41-R67

With this:

options.Environment = _hostingEnvironment.EnvironmentName.ToLower();

While my PR here makes the 3 "default" environments all be lower case, it would be more consistent to have all environment names from the hosting environment be lower case.

Turnerj added 3 commits June 8, 2021 15:36
The staging environment is also built into the framework and thus should be treated the same way as both production and development environments. Other custom environments can still be treated as-is.
@Turnerj Turnerj force-pushed the environment-naming branch from 5467406 to 93f47b9 Compare June 8, 2021 06:09
@bruno-garcia bruno-garcia merged commit 06b8a72 into getsentry:main Jun 10, 2021
@Turnerj Turnerj deleted the environment-naming branch June 13, 2021 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants