Default Environment setting if none provided.

[PureKrome]

Fixes #549.

Required:

• ✔️ set a value to null and make sure `production is in the captured event
• ✔️ set a value to something and make sure it's captures as something, to validate not overwritten.
• ✔️ if aspnetcore doesn't have a test, make sure it does set the right environment, and doesn't break with this change (starts sending production regardless of ASPNETCORE_ENVIRONMENT)

Done:

• 👍🏻 Added check for Environment in SentryEvent and SentryOptions (see below, the reasoning)
• 👍🏻 Added SentryClient tests
• 👀 Checked existing ASP.NET Core tests and they seem to pass.

---

Questions:

A few questions with respect to this PR.

• I've included a SentryOptions check, also. My unskilled understanding is that the SentryEvent might have it set (which is very granular) but if not, then it's possible it was set via SentryOptions (higher up/earlier on) ... which we should then respect.

• I'm not sure I found the scenario's for testing the ASP.NET Core app with respect to testing+environments. I did end up finding this test code, which I tested again (and passed). This test code hinted at the use of setting (and testing) for options (which I first read and referenced here). So I just added a note there, for future developers to get some hints to understanding the code base.

[bruno-garcia]

That was quick @PureKrome ! Thanks a lot.
I left one note though, if you could please address that.

Regarding aspnet core there's a IntegrationTest.cs which could be the way to test it. It boots an app with options, so you could dupe a test and set an environment. Or assert th current one isn't default if that makes sense.

[PureKrome]

there's a IntegrationTest.cs which could be the way to test it

Ah! i see it, i'll give that an 👀 and check it out.

[PureKrome]

@bruno-garcia Okay - found the more appropriate place for this code. I know you mentioned it in the Issue, but I just couldn't find it (global search foo also wasn't working for me 😊 )

What is really important here is that this code stops you from setting the Environment to

• null (e.g. options.Environment = null;)
• string.Empty (e.g. options.Environment = "";)
• strings with only 1+ white spaces (e.g. options.Environment = " ";)

So this means that (from now on) there will always be a value for the options.Environment. Not sure if this is what you want, etc.

[codecov-io]

Codecov Report

Merging #550 into main will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
+ Coverage   85.16%   85.18%   +0.02%     
==========================================
  Files         137      137              
  Lines        3317     3322       +5     
  Branches      749      749              
==========================================
+ Hits         2825     2830       +5     
  Misses        294      294              
  Partials      198      198              

Impacted Files	Coverage Δ
src/Sentry/Internal/MainSentryEventProcessor.cs	95.65% <100.00%> (+0.24%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afbd55e...bb23a82. Read the comment docs.

[bruno-garcia]

Nice and clean change. Thanks.
I did uncover some behavior which I believe isn't correct. If you don't mind, you could fix in this PR. Otherwise we cna merge and have a follow up

[PureKrome]

@bruno-garcia

What is really important here is that this code stops you from setting the Environment to

• null (e.g. options.Environment = null;)
• string.Empty (e.g. options.Environment = "";)
• strings with only 1+ white spaces (e.g. options.Environment = " ";)

So this means that (from now on) there will always be a value for the options.Environment. Not sure if this is what you want, etc.

Can you take a moment to think about this question? I just want to make sure it's been considered.

[bruno-garcia]

@PureKrome you can opt out on BeforeSend since that runs after the event processors

[bruno-garcia]

@PureKrome you can opt out on BeforeSend since that runs after the event processors

To clarify:

SentrySdk.Init(o => o.BeforeSend e => { e.Environment = null; return e; }); this would make sure there's no env set. Right?

[PureKrome]

SentrySdk.Init(o => o.BeforeSend e => { e.Environment = null; return e; }); this would make sure there's no env set. Right?

Ah! gotcha! Yep, I think i noticed the BeforeSend code does get called after the MSEP method does it's stuff. Kewl.

Could this be documented? not sure if the docs need to get updated, at least with some guidence about the default value and order of setting, IMO.

Check the Docs about Environments, I couldn't read anything there that suggests that the Environment setting is required. I'm not sure what the UX would look like, with an empty Environment setting, passed along?

[bruno-garcia]

@PureKrome we've updated the SDK docs for SDK developers to add a default, but user docs wouldn't need to be changed in that way. We could/should document that a default is indeed sent in some SDKs, with the value production. @PeloWriter reached out to discuss this, I'm not sure though what will be the approach. Maybe we'll document one by one? The SDKs that have this default?

[bruno-garcia]

Excellent! Thanks a lot @PureKrome !

[PureKrome]

(I'll leave the docs answer, to others :) )