feat: Add support for ASP.NET Core gRPC #563
Conversation
There was a problem hiding this comment.
There's a lot of good stuff here @Mitch528 thank you.
The general architecture though I believe can't get merged in its current form. It's too much duplication from ASP.NET Core and I think we can redo this in a way that it hooks into the current ASP.NET Core integration. Without having to take over the hook UseSentry. The docs would then refer to: Add Sentry.AspNetCore.Grpc and add AddGrpc to the callback, or add something DI or whatever.
Would you be willing to make the changes necessary?
| _ = services.AddSingleton<ISentryEventProcessor, AspNetCoreEventProcessor>(); | ||
| services.TryAddSingleton<IUserFactory, DefaultUserFactory>(); | ||
|
|
||
| _ = services.AddSingleton<IProtobufRequestPayloadExtractor, DefaultProtobufRequestPayloadExtractor>(); |
There was a problem hiding this comment.
| _ = services.AddSingleton<IProtobufRequestPayloadExtractor, DefaultProtobufRequestPayloadExtractor>(); | |
| _ = services.AddSingleton<IProtobufRequestPayloadExtractor, DefaultProtobufRequestPayloadExtractor>(); |
Looks like instead of copying the whole ASP.NET Core integration, all we need is this line ^, and the one below:
services.AddGrpc(options =>
{
options.Interceptors.Add<SentryGrpcInterceptor>();
});
Besides these, the code that goes in the middleware etc...
This integration could be be done differently then, we could offer a single entrypoint, and add it like:
UseSentry(o => o.AddGrpc()
And through that extension method we find the right hooks, like adding stuff into DI and the middleware.
src/Sentry.Extensions.Protobuf/Sentry.Extensions.Protobuf.csproj
Outdated
Show resolved
Hide resolved
|
@bruno-garcia I think I resolved the issues you had with the code, but let me know if you need me to make any more changes. |
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>netcoreapp3.0;net5.0</TargetFrameworks> |
There was a problem hiding this comment.
Do we really need net5.0 here? Wouldn't netcoreapp3.0 be enough?
There was a problem hiding this comment.
Probably should be good with just netcoreapp3.0. I'll change it.
Codecov Report
@@ Coverage Diff @@
## main #563 +/- ##
===========================================
- Coverage 85.06% 63.18% -21.89%
===========================================
Files 142 176 +34
Lines 3462 4965 +1503
Branches 775 852 +77
===========================================
+ Hits 2945 3137 +192
- Misses 310 1602 +1292
- Partials 207 226 +19
Continue to review full report at Codecov.
|
|
@bruno-garcia Awesome! 🎉 |
No description provided.