Skip to content
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

Extend Trimming to all Sentry packages #2780

Closed
jamescrosswell opened this issue Nov 2, 2023 · 5 comments
Closed

Extend Trimming to all Sentry packages #2780

jamescrosswell opened this issue Nov 2, 2023 · 5 comments
Assignees
Milestone

Comments

@jamescrosswell
Copy link
Collaborator

jamescrosswell commented Nov 2, 2023

In #2732 we added the IsAotCompatible property to Sentry.csproj.

Not planned (legacy or .net framework stuff)

  • src/Sentry.Log4Net/Sentry.Log4Net.csproj
  • src/Sentry.EntityFramework/Sentry.EntityFramework.csproj

Low hanging fruit

Beyond Sentry.csproj many of the other projects can have this added as well with very little modification. We can leverage the same technique to mark these as AOT compatible in the respective csproj files:

  <PropertyGroup Condition="'$(FrameworkSupportsAot)' == 'true'">
    <IsAotCompatible>true</IsAotCompatible>
  </PropertyGroup>

Also, if they don't already, these projects will have to target .net6.0 or later (since FrameworkSupportsAot doesn't have any effect otherwise).

  • IsAotCompatible for uncomplicated projects #2793
    • src/Sentry.Serilog/Sentry.Serilog.csproj
    • src/Sentry.Profiling/Sentry.Profiling.csproj
    • src/Sentry.OpenTelemetry/Sentry.OpenTelemetry.csproj
    • src/Sentry.NLog/Sentry.NLog.csproj
    • src/Sentry.Log4Net/SentryAppender.cs

ConfigureFromConfigurationOptions

Some of our integrations use ConfigureFromConfigurationOptions<TOptions>, which isn't compatible with Trimming and we need to use the new configuration binding source generator:

This affects:

  • Sentry.Extensions.Logging
  • Sentry.Maui
  • Sentry.Azure.Functions.Worker
  • Sentry.AspNetCore

This needs some investigation.

Sentry.DiagnosticSource

We don't need to do anything for Sentry.DiagnosticSource as it's included in the Sentry core package for all versions of .NET that support AOT:

<!-- Sentry.DiagnosticSource is compiled directly into Sentry for .NET Core and .NET targets only. -->
<PropertyGroup Condition="!$(TargetFramework.StartsWith('netstandard')) and !$(TargetFramework.StartsWith('net4'))">
<DefineConstants>$(DefineConstants);HAS_DIAGNOSTIC_INTEGRATION</DefineConstants>
</PropertyGroup>
<ItemGroup Condition="!$(TargetFramework.StartsWith('netstandard')) and !$(TargetFramework.StartsWith('net4'))">
<Compile Include="..\Sentry.DiagnosticSource\Internal\**\*.cs">
<Link>Internal\%(RecursiveDir)%(Filename)%(Extension)</Link>
</Compile>
</ItemGroup>

@bruno-garcia
Copy link
Member

Doesn't really need to be on all.
It should be on stuff that makes most sense. Like Sentry. Sentry.AspNetCore All the serverless stuff.

We don't care about the mostly deprecated stuff (we can change that decision based on feedback later though). Likel Log4Net, EntityFramework, AspNet, etc

@jamescrosswell
Copy link
Collaborator Author

jamescrosswell commented Nov 7, 2023

Doesn't really need to be on all.
It should be on stuff that makes most sense. Like Sentry. Sentry.AspNetCore All the serverless stuff.

I added another section in the issue sescription: "Not planned" and removed Log4Net and EntityFramework from PR2793.

@bitsandfoxes is going to have a got at the ConfigureFromConfigurationOptions stuff, which is the last thing left.

@vaind
Copy link
Collaborator

vaind commented Nov 9, 2023

@jamescrosswell does it make sense to track the progress in this issue's description (as a checklist with project names)?
Also, if it won't require a breaking change, we can just remove this from the 4.0.0 milestone.

@jamescrosswell
Copy link
Collaborator Author

@jamescrosswell does it make sense to track the progress in this issue's description (as a checklist with project names)?
Also, if it won't require a breaking change, we can just remove this from the 4.0.0 milestone.

As far as I know, for all 4 remaining projects it's the same issue (ConfigureFromConfigurationOptions)... so you wouldn't want to work on these separately. At least that's the only issue you get from the compiler - it may be a different story at runtime. At runtime, I think we're likly to hit a few scenarios where additional Json Context needs to be supplied for the sourcegen serializers to be able to serialize everything for each integratino.

We figured it'd be good to spread the knowledge a bit about how this stuff works, so @bitsandfoxes is digging into this...

@jamescrosswell
Copy link
Collaborator Author

Closing in favour of #2822 (as there's quite a bit of context worth capturing and too much in the details of this PR already).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants