fix: Ensure profiling is disabled in Blazor WebAssembly

[alexsohn1126]

THIS PULL REQUEST IS REPLACED BY #4512

Problem

We do not support profiling in Blazor WebAssembly apps. However, it was possible to pass initialize a Blazor WASM app with profiling, which led to undefined behaviours.

The issue above reported failing to publish Blazor WASM app when profiling is enabled. I was not able to replicate that, both in the current version of SDK, and also the version that the issue reporter was using (v4.9.0).

I did find that adding profiling in Blazor WASM led to errors in the browser:

Solution

This pull request does not contain the implementation for enabling Blazor WASM profiling.

That will be handled in this issue:

• Blazor WebAssembly Profiling Support #4506

For now, we will check whether profiling is enabled. If so, we will print a message to the user saying:

And remove profiling integration from our SDK.

This removes the error mentioned above, and hopefully the build error as well.

[jamescrosswell]

Nice - thanks @alexsohn1126 !

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

[Flash0ver]

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.

[Flash0ver]

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?

[alexsohn1126]

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!

[Flash0ver]

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.

[jamescrosswell]

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).

[jamescrosswell]

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);
    }

[alexsohn1126]

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!

[Flash0ver]

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.

[alexsohn1126]

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).

[Flash0ver]

note: needs to be updated after publishing release

• publish: getsentry/[email protected] publish#6257