getsentry / raven-csharp

Superseded by: https://github.com/getsentry/sentry-dotnet
BSD 3-Clause "New" or "Revised" License
231 stars 121 forks source link

Add support for .NET Core #125

Closed polarina closed 6 years ago

polarina commented 8 years ago

Hi,

Is it possible to get the ability to use this library with .NET Core?

Thanks in advance!

gtarsia commented 8 years ago

@polarina I tried porting it to core, but it felt like non-trivial. Looks like the CoreFX project still needs to update some legacy C# stuff for SharpRaven to work (for example: adding Environment.UserName https://github.com/dotnet/corefx/pull/9851).
Maybe someone with more experience on .NET can make a port.

It was thinking of manually sending HTTP requests to sentry to avoid using SharpRaven, but I don't see a tutorial for that. I guess I'll make the javascript report server errors since I'm doing an AspNet site.

sabiland commented 8 years ago

Anything new about .NET Core support?

wakawaka54 commented 8 years ago

Any update on what needs to be done? I'm looking through the codebase now.

asbjornu commented 8 years ago

@wakawaka54 I have no idea, to be honest. I've not started using .NET Core in any projects yet, so I don't know what the requirements are.

timorzadir commented 8 years ago

https://icanhasdot.net/result?github=getsentry~2Fraven-csharp

wakawaka54 commented 8 years ago

Just FYI, I made a .NET Core port.

.NET Core RavenSharp Port

You can add it to your .Net Core project.json file as:

dependencies: {
    "RavenSharp.Core": "1.0.0-beta-1"
}

Then add the configuration to your appsettings.json file as:

  "RavenOptions": {
    "DSN": "dsnfromsentry"
  }

You need to configure your Startup.cs as follows:


        public void ConfigureServices(IServiceCollection services)
{
            services.Configure<RavenOptions>(Configuration.GetSection("RavenOptions"));

            //Add HTTPContextAccessor as Singleton
            services.AddSingleton<IHttpContextAccessor, HttpContextAccessor>();

            //Configure RavenClient
            services.AddScoped<IRavenClient, RavenClient>((s) => {

                var rc = new RavenClient(s.GetRequiredService<IOptions<RavenOptions>>(), s.GetRequiredService<IHttpContextAccessor>())
                {
                    Environment = Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT")
                };
                return rc;
            });
}

You can then inject the IRavenClient interface into where ever you want to use it.

I am not sure how we are going to merge this into the existing RavenSharp code base because I had to rewrite a lot of stuff. The tests had legacy C# stuff in them and the HttpContext stuff has all changed so I had to change all that. But that new port works well for me and I don't have any issues.

ricardoalcantara commented 8 years ago

@wakawaka54 I had the same idea of porting and my code end up very much like yours, but I got very upset tryng to make it compatible with net45, net451, netstandard15 and netstandard16, a lot of things I had comment like you did and I also had to rewrite the Requester, so I thought that it wouldn't be any far for been candidate to get merged. So I decided to create a 'Light' Version myself: RavenSharp Light

I am still working on it and right now I am writing more information at the README.md. I copied the IRavenClient and implemented it all so you could also inject it on top the RavenSharp implementation.

asbjornu commented 8 years ago

@wakawaka54: Awesome work! We can merge your changes into a new branch for now and publish releases from it with a new major version number. Then as we get feedback and feel the .NET Core is stable and sturdy, we can move the develop branch over to it and branch off from the current to a release/2.x branch. Just submit a PR and I'll work out the rest.

sabiland commented 8 years ago

@wakawaka54 great, it works without any problems! :)

ricardoalcantara commented 8 years ago

@asbjornu Cool. Maybe I was wrong about what I thought.

ricardoalcantara commented 8 years ago

@wakawaka54 I would like to help porting the code too, if you need. My fork already compiles to the old .NET (NET45, NET451, NET46) and it also works very much like yours on netstandard1.6. The module part was removed at netstandard1.5, so I had to #if and remove it, so after your PR I could also help with that multi-platform compatibility. maybe!

wakawaka54 commented 8 years ago

@asbjornu Okay a new branch sounds good. I can help rewrite and implement some of the tests I had to comment out. Mostly the tests that use the TestServer stuff doesn't work at all because of all sorts of reasons. But DotNetCore has the new Microsoft.AspNetCore.TestServer stuff which should make that pretty easy to re-implement.

Are you going to make a new branch for me to merge into?

wakawaka54 commented 8 years ago

@ricardoalcantara Okay sure. I like the idea of making a "light" version. However, Sentry.io has some requirements that they like their APIs to meet which is why you have all the Log Scrubbing stuff on there. It would be more of a challenge to get the .Net Core stuff to be backwards compatible with a lot of the older .Net releases, I am not sure if that's worth the extra effort.

ricardoalcantara commented 8 years ago

@wakawaka54 I meant I literally forked the Raven-CSharp . I stopped trying to migrate because I had a lot of troubles with Nancy, and I had to comment few things which I didn't feel comfortable with. I also didn't migrate the configuration you have done.

asbjornu commented 8 years ago

I've created a release/3.0 branch that you can pull request against, @wakawaka54.

jamiecounsell commented 8 years ago

Worth noting: https://blogs.msdn.microsoft.com/dotnet/2016/09/26/introducing-net-standard/

wakawaka54 commented 8 years ago

Yeah that's something to look into. The biggest difficulty that I see with RavenSharp and using .NET Standard is HttpContext. I haven't used the earlier versions of HttpContext but diving into RavenSharp, I saw a lot of stuff that was different between that and .NET Core. If someone could look at the HttpContext in .NET Standard and see if it'll work for both ASP.NET and .NET Core, then adopting it would be straight forward.

sabiland commented 8 years ago

@wakawaka54

Using this configuration:

        public void ConfigureServices(IServiceCollection services)
{
            services.Configure<RavenOptions>(Configuration.GetSection("RavenOptions"));

            //Add HTTPContextAccessor as Singleton
            services.AddSingleton<IHttpContextAccessor, HttpContextAccessor>();

            //Configure RavenClient
            services.AddScoped<IRavenClient, RavenClient>((s) => {

                var rc = new RavenClient(s.GetRequiredService<IOptions<RavenOptions>>(), s.GetRequiredService<IHttpContextAccessor>())
                {
                    Environment = Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT")
                };
                return rc;
            });
}

It seems only 1 sentry message is captured per application start. First capture is sent to Sentry, every another one not. Something wrong with raven services configuration?

EDIT

The problem was this:

I had this version in my project.json "RavenSharp.Core": "1.0.0"

With this one it works ok! "RavenSharp.Core": "1.0.0-beta-1"

consigliory commented 8 years ago

+1

wakawaka54 commented 8 years ago

@sabiland Yeah my bad, I'm a Nuget Noob and didn't think about it too much while I was uploading the package 😞

sabiland commented 8 years ago

@wakawaka54 So it still has to be "RavenSharp.Core": "1.0.0-beta-1" right?

AlexanderNZ commented 8 years ago

Hi all, question for anyone who has got this working,

I've followed @wakawaka54 's instructions, added the snippets in the right place. When I run dotnet build I get

error CS0246: The type or namespace name 'RavenSharp' could not be found (are you missing a using directive or an assembly reference?)

I've added using RavenSharp.Core; to my imports - should I have added something else instead?

asbjornu commented 8 years ago

@AlexanderNZ: I haven't looked at the code and haven't even compiled a .NET Core project yet, but as the name of the project is SharpRaven and not RavenSharp, might that be what the issue here is?

AlexanderNZ commented 8 years ago

I thought that too @asbjornu - I tried SharpRaven.Core initially, but that spawns more errors: https://puu.sh/rECIC/64a765acc3.png

In the end I pulled RavenSharp as that was the dependency name - "RavenSharp.Core": 1.0.0-beta-1

I'm sure I'm just importing wrong, but I can't find any reference to how to import and use RavenSharp.Core. The fact that I'm not a C# guy doesn't help haha

raRaRa commented 7 years ago

Any updates on this? I would really like to use Sentry for my .NET core web application.

asbjornu commented 7 years ago

@raRaRa: We have #161 and #163 that tries to tackle the problem, but none of them are completed. I don't know what @wakawaka54 and @reflectiondm can say about the current status?

LordMike commented 7 years ago

Is there a plan to make implementations of ILogger and friends from Microsoft.Extensions.Logging?

EDIT: I added this as a separate issue.

medokin commented 7 years ago

@wakawaka54 @reflectiondm Friendly bump.

Does anyone work on this, or should it be taken over? With VS 2017 released, the need has raised.

raRaRa commented 7 years ago

Seems like it should be taken over.

reflectiondm commented 7 years ago

@medokin @raRaRa Sorry guys, I have totally no time to dig into it. I also think my PR can be dumped completely and a project should probably aim dotnet standard, thus a different approach should be taken.

asbjornu commented 7 years ago

Anyone who wants to move forward in any direction related to this is more than welcome to submit a PR. 😄

medokin commented 7 years ago

168 looks promising.

@phenixdotnet friendly ping.

wakawaka54 commented 7 years ago

Sorry guys. I just haven't had time to make a formal release. I got this working and it satisfied most of my needs. However, the approach I took is not the greatest. I don't think we can easily merge back into this code base because a lot of the assumptions about HttpContext and other conventions just aren't so anymore. It's crazy how time flies!

Edit: I definitely see a huge demand for this port though. Just my package port has gotten 10,000 downloads over the last 6 months which shows how many people are adopting .NET Core.

wakawaka54 commented 7 years ago

On second thought. I could start taking a look at this sooner. Who wants to pair up with me to finish this once and for all 👍

medokin commented 7 years ago

@wakawaka54 Do you have a todo list what needs to be done?

ricardoalcantara commented 7 years ago

@wakawaka54 I do. As @medokin 's asked: what needs to be done? Also I got lost over the time, what is the branch are you working?

phenixdotnet commented 7 years ago

Hi all. FYI I just update my fork to VS 2017. .NET 3.5 is a problem as it doesn't build with dotnet command but I can work on build script to use msbuild directly.

On the other side if this fork doesn't meet the requirement for a .NET core version I can help a new fresh port. As @medokin and @ricardoalcantara we have to discuss about what should be done and how we can help you.

Regards

wakawaka54 commented 7 years ago

Okay well I've forked over the library and am planning on basically having a separate .NET Core port. At least until a smart chap can come up with a good way to merge the codebases together. I remember the tests were a huge problem because a non .NET Standard Http class was used I believe. I'm going through the codebase now again.

wakawaka54 commented 7 years ago

@medokin @ricardoalcantara @phenixdotnet I don't have a todo list as of right now but I'm looking through the codebase to see where we'll have issues or where we have to change stuff. I really think it'll be easier to just make a solid seperate .NET Core build and then we can see if we want to merge back into the original codebase.

wakawaka54 commented 7 years ago

I do have a question though. The .NET Core HttpContext as well as the Hosting Environment information are available in classes that are outside the .NET Standard framework so we'd have to pick those dependencies up with the .NET Core release. Is this a no-no?

LordMike commented 7 years ago

You mean they're not available, right?

For cases like this, it's best to create two+ projects:

LordMike commented 7 years ago

Acutally. Building on that, the RavenSharp.Core could be the "simple" project which has a client that sends in reports, but doesn't do anything fancy to try to get HTTP details out.

Then, without substituting any implementations, RavenSharp.Core.Asp will provide an ILoggerFactory implementation. It could look like this:

public class RavenLoggerProvider : ILoggerProvider, IDisposable
{
    private readonly IRavenClient _client;
    private readonly LogLevel _minLevel;
    private readonly IHttpContextAccessor _accessor;

    public RavenLoggerProvider(IRavenClient client, LogLevel minLevel, IHttpContextAccessor accessor)
    {
        _client = client;
        _minLevel = minLevel;
        _accessor = accessor;
    }

    public ILogger CreateLogger(string categoryName)
    {
        return new RavenLogger(categoryName, _client, _accessor, _minLevel);
    }

    public void Dispose()
    {
    }
}

It is added to the IServiceProvider like this:

public static class RavenLoggerProviderExtensions
{
    public static ILoggerFactory AddRavenLogger(this ILoggerFactory factory, IRavenClient client, IHttpContextAccessor accessor, LogLevel minLevel = LogLevel.Information)
    {
        factory.AddProvider(new RavenLoggerProvider(client, minLevel, accessor));
        return factory;
    }
}

And then once a message is logged, the IHttpContextAccessor will give you access to the current HttpContext, if one such exists..

I assume the IHttpContextAccessor interface and friends will dissappear in the future, as it is supposedly a compatibility thing.

asbjornu commented 7 years ago

@LordMike: I've been thinking about that sort of refactoring for a long time, so a pull request with only that in it would be merged without blinking. Then it would leave the project in a much better shape for a .NET Core port, won't you agree?

LordMike commented 7 years ago

I kinda cheated with the above snippets and used the same code I used for #175. This client library isn't all too big is it?

Perhaps take it in steps:

I'm bogged down on other endeavours right now. :/

LordMike commented 7 years ago

Sidenote: Another thing that could be given by DI, is a form of enrichment of the sentry report..

Imagine if we did this in the DI framework:

services.Add<IEnricher, MyHttpEnricher>();
services.Add<IEnricher, MyWindowsPrincipalEnricher>();
services.Add<IEnricher, MyRandomEnricher>();

On a report, Raven will then fetch all instances of IEnricher from the DI framework, and run the Enrich(SentryMessage) method on them all (perhaps have a Filter() first).

This way, HTTP specifics is an "Enricher" which is instantiated with the HTTP Accessor, and a principal enricher is one with references to the Microsoft-specific principal libraries.. The core project would only know of the interface.

Anyways. I'm just rambling :)

Note: This might make supporting Windsor Castle, Microsoft DI, AutoFac et. al easier.. (one liners from a users perspective)

asbjornu commented 7 years ago

What I would love to see, is a pull-request ripping SharpRaven.Core apart from everything related to System.Web, without upgrading to VS2017 or anything else. That would be a very low-hanging fruit to merge and would improve all integrations; System.Web, Nancy and a .NET Core port.

wakawaka54 commented 7 years ago

@asbjornu Thats true that would improve future integrations so you would want to target .NET Standard in the future with SharpRaven.Core right? To do that you'll need to get rid of alot more than just System.Web stuff. Would we be able to keep all the legacy stuff like System.Configuration for example which isn't part of .NET Standard in the SharpRaven.Core project and somehow merge the older CSPROJ type to build against .NET Standard and all the other ones? Or will we have to remove System.Configuration and everything not .NET Standard and then break all the legacy implementations like all the Obsolete methods and whatnot?

wakawaka54 commented 7 years ago

@asbjornu This is why I am having trouble making this change easy to merge into the existing codebase because I don't know how you are going to continue to support all the older frameworks. Its easier to just delete everything and start over and just focus on .NET Core. Of course, this is a bad approach because it leaves the libraries disjointed but in the interest of getting the library out for people to use, I'm very tempted to take that route.

asbjornu commented 7 years ago

@wakawaka54:

Thats true that would improve future integrations so you would want to target .NET Standard in the future with SharpRaven.Core right?

I assume so, yes. I haven't invested a single second to attempt understanding the new nomenclature of ".NET Core", but whatever is the smallest sensible build target would be desirable.

To do that you'll need to get rid of alot more than just System.Web stuff.

Ok.

Would we be able to keep all the legacy stuff like System.Configuration for example which isn't part of .NET Standard in the SharpRaven.Core project and somehow merge the older CSPROJ type to build against .NET Standard and all the other ones? Or will we have to remove System.Configuration and everything not .NET Standard and then break all the legacy implementations like all the Obsolete methods and whatnot?

System.Configuration doesn't really need to be referenced. People can provide values from configuration files themselves. So yes, this will be a breaking change that would yield a new major version number, but I think it is warranted and unproblematic.

This is why I am having trouble making this change easy to merge into the existing codebase because I don't know how you are going to continue to support all the older frameworks.

I envision moving everything legacy (that is whatever can't be built with or for .NET Core) out of the Core assembly and hopefully getting Cake to build each part, targeting the intended platforms and frameworks. So that should leave us with something like this:

Assembly Description Targets
SharpRaven.Core The Core of SharpRaven, in an Onion Architecture kind of way. .NET Core1, .NET Framework2
SharpRaven.Nancy As is, basically. Adjusted for the refactored SharpRaven.Core. .NET Core1, .NET Framework2
SharpRaven.AspNet Contains whatever we need to integrate SharpRaven.Core into System.Web.* (ASP.NET) and other legacy .NET Framework thingamajigs. .NET Framework2
SharpRaven.AspNetCore3 Integrates SharpRaven.Core into ASP.NET Core sort of like @LordMike suggests here: https://github.com/getsentry/raven-csharp/issues/125#issuecomment-289531148 .NET Core1
  1. .NET Standard or whatever it is called. Probably with a version number I have no knowledge of.
  2. .NET 3.5, 4.0, 4.5 and 4.6
  3. Tentative naming; please post suggestions.

Thoughts?

phenixdotnet commented 7 years ago

Hi all.

The @asbjornu's proposal looks good, just some points to keep in mind:

Regarding the @LordMike's ideas seem good also but as a user I don't want to have to inject all those dependencies, I want the lib to do it for me at startup if I don't provide my own implementation.