-
-
Notifications
You must be signed in to change notification settings - Fork 210
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
Remove ConfigureFromConfigurationOptions<T> from Sentry.Extensions.Logging #2823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jamescrosswell
changed the title
WIP: Config from config options
Remove ConfigureFromConfigurationOptions<T> from Sentry.Extensions.Logging
Nov 14, 2023
bitsandfoxes
approved these changes
Nov 14, 2023
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'm fine with having this since it's not user-facing or breaking and unblocks us from moving forward. But we really need to figure out a way to source-generate the BindingOptions
once we can confirm this as the working solution.
This was referenced Nov 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#skip-changelog
This change shouldn't impact SDK users at all.
Configure the new source generators
Adding the new source generators is simple enough - we can put something like the following in the respective
*.csproj
files (simplified somewhat):Remove
ConfigureFromConfigurationOptions
classesWe then need to remove the
ConfigureFromConfigurationOptions<TOptions>
classes we're using:sentry-dotnet/src/Sentry.Extensions.Logging/SentryLoggingOptionsSetup.cs
Lines 6 to 12 in 3d0eb8b
These aren't actually doing much. The
SentryLoggingOptionsSetup
here gets anILoggerProviderConfiguration
via dependency injection and uses that for the configuration binding. We can do the same with:sentry-dotnet/src/Sentry.Extensions.Logging/SentryLoggingOptionsSetup.cs
Lines 11 to 26 in 9f616f1
At that point however, we get a bunch of SYSLIB1100 and SYSLIB1101 errors, because the configuration binding source generators don't know how to handle lots of the members of the
SentryOptions
class. It struggles with custom types likeSubstringOrRegex
, it fails completely with members of typeFunc
orAction
and even then, it creates code with a bunch of nullability warnings. In the technical spikes we did, we couldn't overcome any of these when binding to theSentryOptions
type.Solution
The workaround that is used in this PR is to create a much simpler
BindableSentryOptions
class that won't trip up the configuration binding source generators . This class uses nullable primitives likestring?
instead ofSubstringOrRegexPattern
and then takes care of any conversions from those string values to the more complex types we actually use, when applying thatBindableSentryOptions
type to the actualSentryOptions
.sentry-dotnet/src/Sentry.Extensions.Logging/SentryLoggingOptionsSetup.cs
Lines 8 to 26 in 8ca7fd7
For posterity
Some discussions kicked off in the dotnet/runtime repo:
Issues that are relevant to alternative potential solutions in the future: