MindscapeHQ / raygun4net

Raygun provider for .NET
https://raygun.com
MIT License
126 stars 93 forks source link

XRAY-1360 Changes Raygun4Net.AspNetCore to use Raygun4Net.NetCore #386

Closed jscottnz closed 6 years ago

jscottnz commented 6 years ago

This PR adds support for AspNet Core for netstandard1 and netstandard 2 by using the Raygun4Net.NetCore provider.

Importantly - this removes support for net451.

Added a test web project to send errors. ( sample error https://app.raygun.com/crashreporting/14ch10z/errors/2603947097?dateFrom=2018-05-15T21%3A39%3A00.000Z&dateTo=2018-05-22T21%3A39%3A05.000Z )

mduncan26 commented 6 years ago

https://github.com/MindscapeHQ/raygun4net/blob/80dc85a30c4643b7a68c224e31ff03c3329f98d9/Mindscape.Raygun4Net.AspNetCore/Builders/RaygunAspNetCoreMessageBuilder.cs#L100

This should use the same logic as the .NetCore and pickup the version if NetStandard2

https://github.com/MindscapeHQ/raygun4net/blob/80dc85a30c4643b7a68c224e31ff03c3329f98d9/Mindscape.Raygun4Net.NetCore/Builders/RaygunMessageBuilder.cs#L95

mduncan26 commented 6 years ago

There is also two RaygunErrorMessageBuilders (and the base class) the ASP.NET proj should rely on the the common library (.NETCore proj) version

j5alive commented 6 years ago

In general I don't like how this introduces a second RaygunClient and RaygunSettings class with the same names. I can see in your changes lots of needing to prefix these classes with AspNetCore. which I think could be confusing for consumers of this library. E.g. if in my usage of this I try to create a new RaygunClient, which one do I create?

Can we instead create abstract base classes for these called RaygunClientBase and RaygunSettingsBase with all the common parts. We will also need to split all the common message builders and base classes into a shared library, or pull in the required classes from the Mindscape.Raygun4Net.NetCore project via file links and not reference that project at all. @QuantumNightmare may have some more solid guidance on this from how the other provider libraries have been developed.

You may also want to create an example of overriding the DefaultRaygunAspNetCoreClientProvider in your test project.

QuantumNightmare commented 6 years ago

I also find the duplicate RaygunClient and RaygunSettings classes messy and could cause confusion. Another scenario to consider in this discussion:

Can you have an AspNetCore project that references a plain NetCore library project, each having a reference to the appropriate .NET Core Raygun provider? I remember this being an important scenario to consider with the WebApi + plain .NET Raygun providers. There were issues with having 2 namespaces containing classes with the same name. This is why they depend on a common package and have different class names for the RaygunClient.

jscottnz commented 6 years ago

Refactored common code to project: image

image

jscottnz commented 6 years ago

@QuantumNightmare can you elaborate on this?

Can you have an AspNetCore project that references a plain NetCore library project, each having a > > reference to the appropriate .NET Core Raygun provider? I remember this being an important scenario to consider with the WebApi + plain .NET Raygun providers. There were issues with having 2 namespaces containing classes with the same name. This is why they depend on a common package and have different class names for the RaygunClient.

jscottnz commented 6 years ago

@j5alive can you please elaborate on this - what would you be looking for?

You may also want to create an example of overriding the DefaultRaygunAspNetCoreClientProvider in your test project.

j5alive commented 6 years ago

@jscottnz Really just looking for an example of this https://raygun.com/docs/languages/net/aspnetcore#raygun-middleware since you created a test web application it would nice to see this in there too.