Removed obsolete v6 APIs

[jamescrosswell]

Resolves #4608

• Remove Obsolete APIs for 6.0.0 #4608

For discussion

There are a couple of APIs left that are still marked as obsolete, which probably merit some discussion:

Extra

SentrySpan, SentryTransaction and TransactionTracer all have Extra members like:

sentry-dotnet/src/Sentry/SentrySpan.cs

Lines 82 to 84 in 748fcfe

	/// <inheritdoc />
	[Obsolete("Use Data")]
	public IReadOnlyDictionary<string, object?> Extra => Data;

sentry-dotnet/src/Sentry/SentrySpan.cs

Lines 86 to 88 in 748fcfe

	/// <inheritdoc />
	[Obsolete("Use SetData")]
	public void SetExtra(string key, object? value) => SetData(key, value);

Although we do probably want to get rid of these, as believe we also want to get rid of the Data members that we're recommending people switch to... since I think all of these will be replaced by Attributes at some stage right?

Possibly we delay replacing these until the new Attributes are available and, at that point, we could ship an Analyzer and a Code fix to help people migrate (since Extras may be used quite widely).

GetInternalSentryOptions

We also have this, which is a bit of a misuse of the ObsoleteAttribute:

sentry-dotnet/src/Sentry/SentryClientExtensions.cs

Lines 98 to 110 in 216bbc5

	/// <summary>
	/// <para>
	/// Gets internal SentryOptions for integrations like Hangfire that don't support strong assembly names.
	/// </para>
	///<remarks>
	/// *** This is not meant for external use !!! ***
	/// </remarks>>
	/// </summary>
	/// <param name="clientOrHub"></param>
	/// <returns></returns>
	[Obsolete("WARNING: This method is meant for internal usage only")]
	public static SentryOptions? GetInternalSentryOptions(this ISentryClient clientOrHub) =>
	clientOrHub.GetSentryOptions();

We could potentially replace it with this:

[InternalApi("For internal integrations like Hangfire only")]
[EditorBrowsable(EditorBrowsableState.Never)]
public static SentryOptions? GetInternalSentryOptions(this ISentryClient clientOrHub) =>
    clientOrHub.GetSentryOptions();

And then also ship a Roslyn Analyzer that warns if people use something decorated with InternalApiAttribute.

CrashType

Same again for the APIs designed to deliberately create a crash:

sentry-dotnet/src/Sentry/CrashType.cs

Lines 7 to 8 in bd62cf0

	[Obsolete("WARNING: This method deliberately causes a crash, and should not be used in a real application.")]
	public enum CrashType

EnableWatchdogTerminationTracking

Not sure what we want to do with this. Is the long term plan to remove this functionality entirely or are we planning to address the issues with this and "unobsolete" it eventually?

sentry-dotnet/src/Sentry/Platforms/Cocoa/SentryOptions.cs

Lines 147 to 157 in 4a608d4

	/// <summary>
	/// Whether to enable watchdog termination tracking or not. NOT advised.
	/// The default value is <c>false</c> (disabled).
	/// </summary>
	/// <remarks>
	/// This feature is prone to false positives on .NET since it relies on heuristics that don't work in this environment.
	/// </remarks>
	/// <seealso href="https://github.com/getsentry/sentry-dotnet/issues/3860" />
	/// <seealso href="https://docs.sentry.io/platforms/apple/configuration/watchdog-terminations/" />
	[Obsolete("See: https://github.com/getsentry/sentry-dotnet/issues/3860")]
	public bool EnableWatchdogTerminationTracking { get; set; } = false;

In the meantime, do we want to remove it from the .NET API?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (version6@33615f4). Learn more about missing BASE report.

Additional details and impacted files

@@             Coverage Diff             @@
##             version6    #4619   +/-   ##
===========================================
  Coverage            ?   73.04%           
===========================================
  Files               ?      478           
  Lines               ?    17315           
  Branches            ?     3422           
===========================================
  Hits                ?    12647           
  Misses              ?     3813           
  Partials            ?      855           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Flash0ver]

suggestion:

Suggested change

	- Removed obsolete v6 APIs ([#4619](https://github.com/getsentry/sentry-dotnet/pull/4619))
	- Removed obsolete APIs ([#4619](https://github.com/getsentry/sentry-dotnet/pull/4619))

To me, the v6 in the entry doesn't read well ... we haven't added any stable "v6" APIs yet.

[Flash0ver]

re: Extra

• SentrySpan
• SentryTransaction
• TransactionTracer

I do agree on the argument about "consolidating all the Extra, Data, Tags, and such into Attributes".

... we may have an additional problem:

Does this actually also include Sentry.IHasExtra and Sentry.HasExtraExtensions.
If it does, then we can't really remove the Extra indeed, because we haven't marked the IHasExtra interface nor its members, neither have we marked the HasExtraExtensions as obsolete. And also, we have missed the types Sentry.SpanTracer and Sentry.Scope as well:

hub.ConfigureScope(scope =>
{
  scope.SetExtra("Extra!", "Some extra information"); // does not warn
  _ = scope.Extra; // does not warn
});

ITransactionTracer transaction = SentrySdk.StartTransaction("", "");
transaction.SetExtra("", ""); // does not warn
transaction.SetExtras([]); // does not warn
_ = transaction.Extra; // does not warn

ISpan span = SentrySdk.StartSpan("", "");
span.SetExtra("", ""); // does not warn
span.SetExtras([]); // does not warn
_ = span.Extra; // does not warn

_ = (SentrySdk.StartTransaction("", "") as TransactionTracer)!.Extra; // CS0618
_ = (SentrySdk.StartSpan("", "") as SpanTracer)!.Extra; // does not warn

So we have been incomplete with marking the feature "Extra" as Obsolete.
Only if the concrete types SentryTransaction, or SentrySpan or TransactionTracer are used.
But if any interface or other concrete types are used, "Extra" is not marked as Obsolete.

Perhaps we should, for this release, make sure, that we mark "Extra" as Obsolete consistently, including

• IHasExtra interface and members
• HasExtraExtensions type and members
• Scope
• SpanTracer
• ...

Because currently, we only produce on some usages of "Extra" an Obsolete warning, but not on all usages of "Extra", especially on the interfaces which are usually the types that we return.

[Flash0ver]

re: GetInternalSentryOptions

Agreed. Obsolete here is a misuse.
Although I do like something like "InternalApi" more than "Obsolete", this reminds me of Pubternal, which has been banned from the .NET libraries.

Can't we actually make this internal?
...
No, we cannot ... due to the usage in Sentry.Hangfire ... we removed the Strong-Name: #4099

I would love a solution to no longer have this extension method as "Pubternal", but properly "Internal".
But we'd probably want to do this in a separate PR.

In other words:
I vote for leaving this method Obsolete as-is, and think about a "proper" solution, preferably without the use of "Pubternal" through name/namespace/analyzer.

[Flash0ver]

re: CrashType

I do agree ... this is a bit of a misuse of Obsolete ... an Analyzer would be the better fit indeed.
A custom and specific diagnostic that indicates to the user that this method will crash.
This way we can also add a dedicated Help-Link, that the user may click in their IDE that could lead to a documentation page where we describe this and all our future Analyzers.

But I'd like to see this without an additional Attribute.
An Attribute like InternalApi reminds me of Pubternal, a pattern that has been "banned" by Microsoft for the .NET libraries.
Just a Diagnostic-Analyzer, that is enabled per default, reports a Warning per default, and flags all usages of Sentry.CrashType and SentrySdk.CauseCrash.

[Flash0ver]

re: EnableWatchdogTerminationTracking

Hmmm .... I'm uncertain right now 🤔