Skip to content

fix: Ensure profiling is disabled in Blazor WebAssembly #4507

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

fix: Ensure profiling is disabled in Blazor WebAssembly #4507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
804cd0f
fix: Ensure profiling disabled in Blazor WASM
alexsohn1126 Sep 9, 2025
1adda76
clarify wording in debug log
alexsohn1126 Sep 9, 2025
7347afa
fix spaces between sentences
alexsohn1126 Sep 9, 2025
3825367
Add disabling Blazor WASM profiling in CHANGELOG.md
alexsohn1126 Sep 9, 2025
6450f44
say WebAssembly of WASM in changelog.md
alexsohn1126 Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
- `InvalidOperationException` potentially thrown during a race condition, especially in concurrent high-volume logging scenarios ([#4428](https://github.com/getsentry/sentry-dotnet/pull/4428))
- Blocking calls are no longer treated as unhandled crashes ([#4458](https://github.com/getsentry/sentry-dotnet/pull/4458))
- Only apply Session Replay masks to specific control types when necessary to avoid performance issues in MAUI apps with complex UIs ([#4445](https://github.com/getsentry/sentry-dotnet/pull/4445))
- Ensure profiling is disabled in Blazor WebAssembly ([#4507](https://github.com/getsentry/sentry-dotnet/pull/4507))
Copy link
Member

@Flash0ver Flash0ver Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: needs to be updated after publishing release


### Dependencies

Expand Down
1 change: 1 addition & 0 deletions src/Sentry.AspNetCore.Blazor.WebAssembly/Sentry.AspNetCore.Blazor.WebAssembly.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

<ItemGroup>
<ProjectReference Include="..\Sentry.Extensions.Logging\Sentry.Extensions.Logging.csproj" />
<ProjectReference Include="..\Sentry.Profiling\Sentry.Profiling.csproj" />
Copy link
Member

@Flash0ver Flash0ver Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: new dependency

I'm a bit hesitant of adding this new dependency, as we might paint ourselves into a (potential) breaking change corner in the future:
Unless I see that wrong, we wouldn't implement #4506 in Sentry.Profiling, but instead in Sentry.AspNetCore.Blazor.WebAssembly, or a new WASM-Profiling package, which would/could result into removing this dependency again ... so we could, strictly speaking, only change it in a major release.
Well ... now that I think about it again ... since Sentry.Profiling is not supported in Sentry.AspNetCore.Blazor.WebAssembly currently anyway, later removing Sentry.Profiling isn't actually really a breaking change.
So perhaps this is a non-issue.

The other thought that I had is that adding this dependency is increasing the size of the consuming Application, which especially for WASM apps is a concern where bundle sizes and download speeds to the Browser are worth a consideration ... because now every WASM app will depend on Sentry.Profiling, regardless of whether it actually tries to add the Profiling integration or not.
Which again isn't really a blocking issue either ... more of a "not-so-nice-to-have" thought.

Copy link
Member

@Flash0ver Flash0ver Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if there is another way to signal and/or force-disable Profiling for WASM-apps 🤔.

  • docs
    • we do mention that "Blazor WebAssembly" is not supported in our Profiling-Troubleshooting already
  • MSBuild warning
    • I wonder if we could ship a custom Target in Sentry.Profiling that produces a build warning when Sentry.Profiling is used in a WASM app, and suggests to the user to remove the dependency again since it's currently not supported
  • make SentryOptionsProfilingExtensions.AddProfilingIntegration and/or ProfilingIntegration.Register no-op and emitting the Diagnostic-Log if the SentryOptions type is SentryBlazorOptions
    • Sentry.AspNetCore.Blazor.WebAssembly has it's own SentryBlazorOptions type. We could instead move that check to Sentry.Profiling, to keep it "contained"
    • well to be fair ... this wouldn't be a Type-check due to the lacking dependency, but instead be a String-Type-Name check. So we should definitely have a unit test around that behavior. Not the most aesthetic solution either, but this way, we wouldn't "pessimize" every WASM project that doesn't even try to enable Profiling by now also including the dependency
  • ...

Hmmm ... perhaps there is another, more elegant solution.

What do you think?

Copy link
Member Author

@alexsohn1126 alexsohn1126 Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great points Stefan, I'll go over some of the points that you made:

I'm a bit hesitant of adding this new dependency, as we might paint ourselves into a (potential) breaking change corner in the future

Yes, I was also hesitant to make this dependency, since we really only use Sentry.Profiling to get the ProfilingIntegration type in line 52. It is also weird that we'd be adding profiling to a platform that we know that doesn't support profiling. I submitted this solution because I couldn't really think of another way to use RemoveIntegration function without importing the ProfilingIntegration type.

MSBuild warning

I think the MSBuild warning is an interesting idea, we already do have custom Targets in Sentry.Profiling that errors out for IOS and Android platforms:

sentry-dotnet/src/Sentry.Profiling/buildTransitive/Sentry.Profiling.targets

Lines 6 to 11 in 1a6d7e7

<Target BeforeTargets="BeforeBuild" Name="CheckPlatformSupport">
<Error Text="Package Sentry.Profiling is not supported on platform '$(TargetPlatformIdentifier)'."
Condition="'$(TargetPlatformIdentifier)' == 'android'" />
<Error Text="Package Sentry.Profiling is not supported on platform '$(TargetPlatformIdentifier)'. The profiler is already part of the main Sentry package."
Condition="'$(TargetPlatformIdentifier)' == 'ios' or '$(TargetPlatformIdentifier)' == 'maccatalyst'" />
</Target>

However, from a chat with @jamescrosswell, I learned that WASM doesn't really have a platform of it's own since it runs on the browser. So it will have to check for something entirely different, not platform.

I will try to explore more about custom Targets and how we can possibly use them in this context.

make SentryOptionsProfilingExtensions.AddProfilingIntegration and/or ProfilingIntegration.Register no-op and emitting the Diagnostic-Log if the SentryOptions type is SentryBlazorOptions

Yes, this would definitely be a valid solution, and would keep the dependency away from the WASM project. I'll use this as a fallback in case I can't solve this problem with MSBuild solution that you mentioned. While I also do not particularly like the raw string check with types, I don't think there's a way around it, and it sounds like the lesser evil than creating another dependency.

I'll definitely add tests around that if I do end up going down this route!

Copy link
Member

@Flash0ver Flash0ver Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm ... re MSBuild ... your thoughts have brainstormed me into:
In the BCL, I noticed usages of [SupportedOSPlatform("browser")] and [UnsupportedOSPlatform("browser")], and I believe there is also a BROWSER preprocessor symbol.
That makes me wonder if there actually is a TargetPlatformIdentifier == 'browser' defined for WASM 🤔

checked grep.app ... the .NET Runtime has some usages
https://github.com/dotnet/runtime/blob/dcf86807937706d8d961a4aff0b98a8f55b563ba/src/libraries/System.Runtime.InteropServices.JavaScript/src/System.Runtime.InteropServices.JavaScript.csproj#L12-L15

If that works, it would be then a consistent solution with android indeed.

Copy link
Collaborator

@jamescrosswell jamescrosswell Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed this additional dependency when chatting with @Flash0ver last night...

@alexsohn1126 I agree it's pretty weird to add a dependency on Profiling just to call a method that removes the profiling integration. This could potentially increase the size of our WASM integration (which would increase the size of the resulting WASM assemblies that our SDK users ship).

I think we either find another way to detect whether someone has tried to enable profiling in WASM or we don't add this logic (we just leave it in the troubleshooting section of the docs instead).

Copy link
Collaborator

@jamescrosswell jamescrosswell Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option might be to find a way to do this differently (so that we don't need to have access to the ProfilingIntegration type in order to remove that integration type):

sentry-dotnet/src/Sentry/SentryOptions.cs

Lines 1342 to 1346 in eadf5f2

public void RemoveIntegration<TIntegration>() where TIntegration : ISdkIntegration
{
// Note: Not removing default integrations
_integrations.RemoveAll(integration => integration is TIntegration);
}

We could, for example, add an internal INamedSdkIntegration and have the ProfilingIntegration implement this. Then we could do something like:

    internal void RemoveIntegrationByName(string name)
    {
        _integrations.RemoveAll(integration => integration is INamedSdkIntegration namedIntegration && namedIntegration.Name == name);
    }

Copy link
Member Author

@alexsohn1126 alexsohn1126 Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I've tested so far, TargetPlatformIdentifier is not set when you in a Blazor WebAssembly application. If I try to print it out pre-build, it just prints a blank string, so I don't think that is possible.

I did find out we can use UsingMicrosoftNETSdkBlazorWebAssembly property:
https://github.com/dotnet/sdk/blob/main/src/BlazorWasmSdk/Sdk/Sdk.props#L14

I will make a change to the pull request whenever I can confirm that we can stop a user from building a wasm app with profiling. Though this would mean that they will be blocked from building the app which may not be the best experience. Then again, it makes sense to just not allow the user to use profiling in the first place if we don't support it 🤷‍♂️

If the method mentioned above doesn't work, what @jamescrosswell suggested looks pretty clean!

<ProjectReference Include="..\Sentry\Sentry.csproj" />
</ItemGroup>

Expand Down
19 changes: 19 additions & 0 deletions src/Sentry.AspNetCore.Blazor.WebAssembly/WebAssemblyHostBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using Microsoft.Extensions.Logging;
using Sentry;
using Sentry.Extensibility;
using Sentry.Extensions.Logging;
using Sentry.Profiling;

// ReSharper disable once CheckNamespace - Discoverability
namespace Microsoft.AspNetCore.Components.WebAssembly.Hosting;
Expand Down Expand Up @@ -28,10 +30,27 @@ public static WebAssemblyHostBuilder UseSentry(this WebAssemblyHostBuilder build
blazorOptions.RequestBodyCompressionLevel = CompressionLevel.NoCompression;
// Since the WebAssemblyHost is a client-side application
blazorOptions.IsGlobalModeEnabled = true;
// If profiling enabled, disable it.
RemoveBlazorProfilingIntegration(blazorOptions);
});

return builder;
}

private static void RemoveBlazorProfilingIntegration(SentryBlazorOptions options)
{
if (!options.IsProfilingEnabled)
{
return;
}

options.SetupLogging();
options.LogDebug("Detected Sentry profiling initialization in Blazor WebAssembly. " +
Copy link
Member

@Flash0ver Flash0ver Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: level

Since we are force-disabling an explicit opt-in from user code, I wonder if this may be of a higher Log-Level. Perhaps even Warning.
And it's only logged once per app lifetime, it wouldn't be too noisy, yet strongly suggesting to remove the Sentry.Profiling dependency from the consuming application, as it's currently not supported.

Copy link
Member Author

@alexsohn1126 alexsohn1126 Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about putting this message under warning level as well, so I will change the log to be a warning if I don't figure out how to make MSBuild solution work (mentioned above).

"Sentry does not support Blazor WebAssembly profiling. Removing profiling integration. " +
"Check https://github.com/getsentry/sentry-dotnet/issues/4506 for more information.");
// Ensure project doesn't have Profiling Integration
options.RemoveIntegration<ProfilingIntegration>();
Copy link
Collaborator

@jamescrosswell jamescrosswell Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice - thanks @alexsohn1126 !

We could also add a unit test to SentryWebHostBuilderExtensionsTests to make sure that the integration gets removed.

}
}

/// <summary>
Expand Down
3 changes: 2 additions & 1 deletion src/Sentry/Sentry.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<TargetFrameworks Condition="'$(NO_IOS)' == '' And $([MSBuild]::IsOSPlatform('OSX'))">$(TargetFrameworks);net9.0-ios18.0;net8.0-ios17.0</TargetFrameworks>
<TargetFrameworks Condition="'$(NO_MACCATALYST)' == '' And $([MSBuild]::IsOSPlatform('OSX'))">$(TargetFrameworks);net9.0-maccatalyst18.0;net8.0-maccatalyst17.0</TargetFrameworks>
</PropertyGroup>

<!-- Platform-specific props included here -->
<Import Project="Platforms\Android\Sentry.Android.props" Condition="'$(TargetPlatformIdentifier)' == 'android'" />
<Import Project="Platforms\Cocoa\Sentry.Cocoa.props" Condition="'$(TargetPlatformIdentifier)' == 'ios' Or '$(TargetPlatformIdentifier)' == 'maccatalyst'" />
Expand Down Expand Up @@ -148,6 +148,7 @@
<InternalsVisibleTo Include="Sentry.AspNetCore.Grpc.Tests" PublicKey="$(SentryPublicKey)" />
<InternalsVisibleTo Include="Sentry.AspNetCore.Tests" PublicKey="$(SentryPublicKey)" />
<InternalsVisibleTo Include="Sentry.AspNetCore.TestUtils" PublicKey="$(SentryPublicKey)" />
<InternalsVisibleTo Include="Sentry.AspNetCore.Blazor.WebAssembly" PublicKey="$(SentryPublicKey)" />
<InternalsVisibleTo Include="Sentry.Azure.Functions.Worker" PublicKey="$(SentryPublicKey)" />
<InternalsVisibleTo Include="Sentry.Azure.Functions.Worker.Tests" PublicKey="$(SentryPublicKey)" />
<InternalsVisibleTo Include="Sentry.Benchmarks" PublicKey="$(SentryPublicKey)" />
Expand Down
Loading