Adding Sentry to global usings breaks compilation all over the place in 4.0.0

[frankbuckley]

Package

Sentry

.NET Flavor

.NET

.NET Version

8.0.1

OS

Any (not platform specific)

SDK Version

4.0.0

Self-Hosted Sentry Version

No response

Steps to Reproduce

Upgrade to 4.0.0. Try to compile.

Expected Result

Compilation succeeds. Maybe a couple of tweaks for a version change.

Actual Result

Compilation errors all over the place.

This clashes all over the place for me (Request, Session, Attachment,...) - and let alone our own types, obvious core library types - e.g.:

error CS0104: 'ISession' is an ambiguous reference between 'Microsoft.AspNetCore.Http.ISession' and 'Sentry.ISession'

and...

error CS0104: 'Session' is an ambiguous reference between 'Stripe.Checkout.Session' and 'Sentry.Session'
error CS0104: 'Package' is an ambiguous reference between 'Windows.ApplicationModel.Package' and 'Sentry.Package'

It is a huge assumption to think that library users want using Sentry injected into every source file in their projects!

Can you explain the thinking here?

We usually only ever interact with Sentry types in startup/configuration code (1 or 2 source files in a solution). It is difficult to see why using Sentry; is being equated with the prevalence of using System.Collections.Generic;?

[jamescrosswell]

Hm, maybe we jumped the gun a bit on this. @bitsandfoxes / @bruno-garcia should we revert to opt-in rather than opt-out for global usings?

[jhartmann123]

Got the same issue two years ago in 3.14, where it was reverted. The decision back then was to rename potentially conflicting names - but seeing conflicts on very generic names like ISession, Session, Package it doesn't look like that that plan worked.

#1487

I still think that adding Sentry to the global usings by default is a bad idea, as probably 90% of the users of this lib call it exactly once - when configuring it.

The workaround for now is by opting out of the global usings, as noted in the release notes:

<PropertyGroup>
  <SentryImplicitUsings>false</SentryImplicitUsings>
</PropertyGroup>

[frankbuckley]

Got the same issue two years ago in 3.14, where it was reverted. The decision back then was to rename potentially conflicting names - but seeing conflicts on very generic names like ISession, Session, Package it doesn't look like that that plan worked.

#1487

I still think that adding Sentry to the global usings by default is a bad idea, as probably 90% of the users of this lib call it exactly once - when configuring it.

Agree.

Indeed, I do not think any library should pollute its users' global namespaces by default (with, perhaps, the exception of some core BCL namespaces). Offer the option, if you want, but do not make it the default.

That said, I am not really sure what a <SentryImplicitUsings> property offers over the (existing and understood) <Using Include="Sentry" /> for those who want it. Most would miss it (if off by default), it needs documenting and maintaining. Why bother?

Seems like the wrong solution to a non-problem.

[bitsandfoxes]

The idea was to minimize any friction between installation and initialization and make it ready to inspect the API from the get-go.

Seems like the wrong solution to a non-problem.

Fair enough. Sorry about that.

[bruno-garcia]

Seems like the wrong solution to a non-problem.

Believe it or not we've received support tickets in the past with "I get an error SentrySdk isnt' found" etc. For that reason we add to the docs everywhere the using needed. But agree that C# is usually pretty good here (with VS or Rider) suggesting using directives or even which packages might include that type.

True that we missed some types very likely to clash here and worth addressing this asap but before rolling it back a second time, worth taking a stab at just fixing the conflicts.

That said, I am not really sure what a property offers over the (existing and understood)

It allows you to quickly disable this change and avoid the breakage we introduced to you. Ideally no one would be aware of this unless needed to adopt the package and avoid conflicts (which could come in handy now).

[bitsandfoxes]

Hey @frankbuckley, @jhartmann123.
I'm perfectly happy to roll back the change if necessary, but first, I'd like to give this another try. We've introduced a number of breaking changes specifically for that feature, and reverting them now without trying to fix it first would be a bit of a waste. The new version is supposed to take care of the remaining conflicting types, so let's see if we can troubleshoot and resolve the issues before considering a rollback.
Please let me know if you're still running into any issues with the 4.0.1

[nesherhh]

We still have issues with 4.0.1. This time it is Request class.
I think you will always find some class names that will conflict with the existing code.

[frankbuckley]

We still have issues with 4.0.1. This time it is Request class.
I think you will always find some class names that will conflict with the existing code.

Yup, still got that one with 4.0.1:

error CS0104: 'Request' is an ambiguous reference between 'DSE.Identity.Domain.Model.Requests.Request' and 'Sentry.Request'

[frankbuckley]

Please let me know if you're still running into any issues with the 4.0.1

error CS0104: 'Package' is an ambiguous reference between 'Windows.ApplicationModel.Package' and 'Sentry.Package' 

[bruno-garcia]

4.0.2 is out with those types renamed. Thanks again for raising this and for bearing with us folks.
Anything causing conflict?

[LionelVallet]

Constants is causing conflict too.

[bitsandfoxes]

My sincerest apologies and thanks for pointing these out. I missed those, that's on me.