feat: options.AddProfilingIntegration()

[vaind]

No description provided.

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against bd53758

[bruno-garcia]

don't have the full picture anymore but could we add the integration automatically if the sample rates was set? I wonder if it could help with the iOS situation where this shouldn't be called

[vaind]

don't have the full picture anymore but could we add the integration automatically if the sample rates was set? I wonder if it could help with the iOS situation where this shouldn't be called

have you seen anything actually useful in there?

I guess we could parse the build.log and add it as a job summary, although I'm not sure how it behaves with the existing summaries coming from .net tests. I understand they would overwrite one-another...

[bruno-garcia]

@vaind I think regardless of that we should add this in and change the docs. Much nicer API than the current approacbh

[vaind]

actually I've pasted that comment in the wrong PR sorry for closing this

[vaind]

don't have the full picture anymore but could we add the integration automatically if the sample rates was set? I wonder if it could help with the iOS situation where this shouldn't be called

To really answer your question: I don't know if/how we could do that. The integration is defined in another package so we can't directly reference it in the main Sentry package. I've thought about making the actual ProfilesSampleRate option an extension defined in the profiling package - that would achieve the desired behavior you're describing - but it seems like c# doesn't support property extensions. A partial class is also not an option because they're only allowed to be defined in the same assembly. Any other ideas?

[bruno-garcia]

don't have the full picture anymore but could we add the integration automatically if the sample rates was set? I wonder if it could help with the iOS situation where this shouldn't be called

To really answer your question: I don't know if/how we could do that. The integration is defined in another package so we can't directly reference it in the main Sentry package. I've thought about making the actual ProfilesSampleRate option an extension defined in the profiling package - that would achieve the desired behavior you're describing - but it seems like c# doesn't support property extensions. A partial class is also not an option because they're only allowed to be defined in the same assembly. Any other ideas?

It would require reflection.
But nevermind, it's not worth it

[bruno-garcia]

We could though #if IOS here and have a no-op version?
"on iOS we use a native profiler"

Then it wouldn't matter during Set Up if we say AddProfilerIntegration on all platforms. It would just no-op on iOS?

#if IOS
// debug log?
return;
#endif

[vaind]

how about 5abd66d

[bruno-garcia]

Nice! that looks good

[bruno-garcia]

Wondering if a warning is needed. Because if I have a shared Init logic for Sentry by used Apps, I just want profiling to be there. the fact iOS has it automatically but not the other platforms is irrelevant.

Suggested change

	DiagnosticLogger?.LogWarning($"{nameof(ProfilingIntegration)} has already been added. The second call to {nameof(AddProfilingIntegration)} will be ignored.");
	DiagnosticLogger?.LogDebug($"{nameof(ProfilingIntegration)} on iOS is added automatically. The second call to {nameof(AddProfilingIntegration)} will be ignored.");

[vaind]

I've added the warning (actually the exact line of code) from the other profiling integration. Arguably, it may be unnecessary to warn when you add the same thing multiple times, no clue why integrations have warnings about that.

Because if I have a shared Init logic for Sentry by used Apps, I just want profiling to be there. the fact iOS has it automatically but not the other platforms is irrelevant.

Doesn't play a role here. The warning only triggers when a user added the integration manually multiple times. Not when they add it once after we've added it automatically during init.

I'll go ahead and merge this as is so it's exactly the same as the other profiling integration (which also warns only when you add it manually multiple times). We can change it later in all the places with he same LogWarning if you think we should