-
-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add IHostBuilder support #1015
Add IHostBuilder support #1015
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1015 +/- ##
==========================================
+ Coverage 81.43% 81.45% +0.01%
==========================================
Files 193 195 +2
Lines 6325 6348 +23
Branches 1526 1537 +11
==========================================
+ Hits 5151 5171 +20
- Misses 739 740 +1
- Partials 435 437 +2
Continue to review full report at Codecov.
|
Can we converge on just one way of using |
What exactly do you mean ? |
Co-authored-by: Alexey Golub <[email protected]>
Sorry. I was referring to how having both the option to put |
I'm not sure we can do anything about that. I understand it could be confusing. One way I suggested to 'tackle' that problem was explained in the original issue
But that was also not desirable behaviour |
Let's see what @bruno-garcia thinks about this |
Not a fan of the two methods either. |
My 2 cents, so you can make an informed decision
That could indeed be an option, deprecate IWebHostBuilder extensions. While both still seem acceptable ways to create a Host from asp.net core's standpoint. There does not seem to be plans to deprecate/remove
As far as I know, both of these are equivalent.
I can't see anything that you cannot achieve through the But don't take my word for it :) |
Sounds like we do need to keep both. So one way is: We add the infrastructure for We could add a note that All of that assumes that we can make them equivalent. If we can add "more" features through |
Exactly, at the time of writing, the current |
Sounds like we could pull the code to a single place and forward the call from both places then? Is the code reentrant already? We can expect that ppl will call it twice (from each interface). |
I looked into that, but IHostBuilder and IWebHostBuilder are not the same interface, so code sharing is not easily done
No not yet, I will comment in code where I see issues |
|
||
_ = logging.Services | ||
.AddSingleton<IConfigureOptions<SentryAspNetCoreOptions>, SentryAspNetCoreOptionsSetup>(); | ||
_ = logging.Services.AddSingleton<ILoggerProvider, SentryAspNetCoreLoggerProvider>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bruno-garcia using TryAddSingleton here can cause issues, as it won't register it if let's say Serilog or other already exists, in this case you want the 'override' ability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. Is there a way to check if the specific specification has been added? I believe it was possible to check the items in the collection so we can verify if specifically SentryAspNetCoreLoggerProvider
was already added and make sure we only add it once.
.AddSingleton<IConfigureOptions<SentryAspNetCoreOptions>, SentryAspNetCoreOptionsSetup>(); | ||
_ = logging.Services.AddSingleton<ILoggerProvider, SentryAspNetCoreLoggerProvider>(); | ||
|
||
_ = logging.AddFilter<SentryAspNetCoreLoggerProvider>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no TryAddFilter, i'm not sure it is an issue if this is added twice thought
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure either, I guess something we'd need to test? There's no way to look into the filters already added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a closer look at it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it boils down to:
So it does not really seem to matter much
configureSentry?.Invoke(context, sentryBuilder); | ||
}); | ||
|
||
_ = builder.ConfigureServices((c, s) => _ = s.AddTransient<IStartupFilter, SentryStartupFilter>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TryAddTransient is also an issue here, if any IStartupFilter was already added (which most likely it will) this will not be registered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something lke:
if (!s.Any(d => d.ServiceType == descriptor.ServiceType && d.GetImplementationType() == implementationType))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice, I didn't think of that.
Just ran a quick test and yes that would work.
I dropped some comments here and there (see above) In some code bases, I worked around this with a |
This strategy won't work in the ASP.NET Core case because even on a normal run of it, 2 containers are created and the first time we'd add (before the static fields are set) would end up on a dead host, and the new one wouldn't have it. |
Took the comments into consideration, made
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just a nitpick but this already looks good to me!
Before we merge, It would be nice to have some coverage of this by some test, even an integration test of sorts.
I assume we have such test in the ASP.NET Core integration already then it could be copied over since it's the same thing.
It could also be used from one of the samples (Samples.ME.Logging or GenericHost actually)?
The samples make sure we have an easy way to run it and verify it works. And also to 'show ppl' some examples. And to have usage of the API to see what it looks like.
/// <returns></returns> | ||
internal static IServiceCollection AddSentryStartupFilter(this IServiceCollection serviceCollection) | ||
{ | ||
if(!serviceCollection.IsRegistered<IStartupFilter, SentryStartupFilter>()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: one day we'll have a linter that reformats and pushed to the branch :)
Could you please do this throughout the change?
if(!serviceCollection.IsRegistered<IStartupFilter, SentryStartupFilter>()) | |
if (!serviceCollection.IsRegistered<IStartupFilter, SentryStartupFilter>()) |
Latest commit should've fixed failing tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this feature! There are a few minor things to address.
I'd even say it's likely good enough to merge and we can adjust things on our end. What would you say @Tyrrrz ?
@@ -0,0 +1,70 @@ | |||
using System; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer we remove this file. The idea of Basic
is to have the simplest possible example.
I'd prefer we continued to use a single file for this example. We can even refactor this to use top level statements.
We can remove the WebHost example and use only your new approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of the current version of basic ?
l.AddSentry(); | ||
}) | ||
.ConfigureLogging(b => b.AddConsole()) | ||
.UseSentry() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
@@ -12,15 +12,15 @@ | |||
</ItemGroup> | |||
|
|||
<ItemGroup> | |||
<ProjectReference Include="../../src/Sentry.Extensions.Logging/Sentry.Extensions.Logging.csproj" /> | |||
<ProjectReference Include="../../src/Sentry.AspNetCore/Sentry.AspNetCore.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to change the dependency to ASP.NET Core integration? The new types are all on the S.E.L, right?
<ProjectReference Include="../../src/Sentry.AspNetCore/Sentry.AspNetCore.csproj" /> | |
<ProjectReference Include="../../src/Sentry.Extensions.Logging/Sentry.Extensions.Logging.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New types are not on S.E.L. right now, see comment below
/// </summary> | ||
/// <param name="builder">The builder.</param> | ||
/// <returns></returns> | ||
public static IHostBuilder UseSentry(this IHostBuilder builder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IHostBuilder
lives in Microsoft.Extensions.Hosting
so this could be added to Sentry.Extensions.Logging
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would've personally kept it in the asp.net core part, as it is the replacement of WebHostBuilder in the future.
Would you like me to move it ?
Co-authored-by: Bruno Garcia <[email protected]>
I would also prefer to avoid having a dependency on |
I understand How do we proceed with these dependencies:
|
I'm also okay with merging as is since the PR has been up for a while, and then improving on it as we go along 😉 |
I'll leave that up to you guys |
Could we move all of that to
Instead of AspNetCore.Hosting, isn't this So it's either that or a new |
I'll take a closer look tomorrow |
Well I couldn't keep myself, took a quick gander. Things like:
All require AspNetCore ref I believe you won't be able to substitute
I don't see anything wrong with that, if it's clear from a usage point of view. |
@bruno-garcia Going forward, I believe a new Do you wish for me to proceed on that path ? |
Yeah sounds like we'll need it particularly because we're looking at adding support to MAUI as soon as it's out so we'd really appreciate your help with this!
Please take a look at the other libraries like |
Ok, thank you for the feedback. I might have some time this week to continue working on this |
I don't have anything to add about how this should be implemented but I just wanted to voice my support for adding this. The above describes my use case exactly, as I use |
Thanks again for the time spent here, but we've decided not to do this. See #190 (comment) Thank you. |
As discussed here: #1001
First quick and dirty commit
My first thought after beginning to add an implementation: would it be bad if it acted 100% like the IWebHostBuilder extension.
The user would have the choice between:
And
Only 'caveat' right now is that it would add a few classes that would never be used in a WorkerService environment:
However i'm not sure this is such an issue, thoughts ?
Also, I haven't taken #1001 (comment) into consideration