Fixes for 8.0.0

[philipphofmann]

Ideas for fixes in 8.0.0.

[philipphofmann]

Rename SentryDebugMeta to SentryDebugImage and add a new SentryDebugMeta that aligns with the DebugMeta from the unified API.

[armcknight]

Mark some properties in SentryEvent.h readonly to avoid pointer modification by consumers. Then we can remove the scary warnings in the headerdocs like this:

sentry-cocoa/Sources/Sentry/Public/SentryEvent.h

Lines 62 to 63 in ce1b8d1

	* This property will be filled before the event is sent. Do not change it
	* otherwise you know what you are doing.

[armcknight]

SentryTransactionContext is the only subclass of SentrySpanContext. They should be combined into a single class.

[armcknight]

Same with SentryTransaction, which is the sole subclass of SentryEvent.

[armcknight]

Given both of these I'd say we should end with a SentryTransaction (subsuming SentryEvent) and SentryTransactionContext (subsuming SentrySpanContext).

[kevinrenskers]

SentryTransactionContext is the only subclass of SentrySpanContext, but they are both being used, right? So combining them into a single class would add properties to SentrySpanContext that maybe shouldn't be there?

[armcknight]

Yes, you are right, SentrySpanContext is used in -[SentryTracer startChildWithParentId...]. I can't tell from the various places they're both used how they overlap or differ, so this can just stay as is.

[armcknight]

SentrySamplingContext.customSamplingContext is never read by anything to send to the backend, nor to alter the SDK's behavior, so should be removed.

[kevinrenskers]

It can be used by the user of the SDK by setting profilesSampler in SentryOptions. This is used to dynamically determine the sample rate, and that function can then use the custom context. But wow that is a lot of code involved just for this one feature 😅

[armcknight]

I see it now, thanks for pointing that out. It looks like customSamplingContext always appears alongside a SentryTransactionContext, which I assume is also the same one a SentrySamplingContext references, and consumers that implement tracesSampler have access to the transaction context via the property on the sampling context, so they probably could be combined, but it's not a huge deal.

[armcknight]

We should not expose SentryOptions.diagnosticLevel for public consumption/modification. Logging should be either info/error as the default, or debug of the SDK debug mode is enabled.

[kevinrenskers]

Strongly disagree with this. "Logging should be either info/error as the default" - therein lies the problem. Which one do you pick? Neither one will be the right choice for 100% of people. And even while working on an app in debug mode, the debug level can simply be too noisy. I really think this needs to be configurable by users of the SDK. It's a fundamental option that basically all SDKs offer.

[armcknight]

We code SentryBreadcrumb.type as an NSString with a comment that it should only be one of a given set of values

Type of breadcrumb, can be e.g.: http, empty, user, navigation

However we have other types listed at https://develop.sentry.dev/sdk/event-payloads/breadcrumbs/#breadcrumb-types

Instead of leaving this seemingly open-ended (what happens if consumers provide a value that's not in the list? The above doc doesn't say, nor does https://docs.sentry.io/platforms/apple/enriching-events/breadcrumbs/) with a comment on what is valid (can't be automatically checked and drifts from implementation), we should provide an enum of valid values.

We should probably also validate that the data being provided to a breadcrumb matches the type of breadcrumb it is, logging a warning for any unexpected field keys.

[armcknight]

Just checked the Android SDK to see if it's done differently in another SDK and it is implemented the same way, so I'm digging for further requirements on this. See getsentry/sentry#37674

[philipphofmann]

Yes, there are a few issues tagged with 8.0.0, but is there a significant one worth shipping right now? I rather wait as long as we can to avoid breaking changes.

[kevinrenskers]

What do you mean with this? We already support apps built with Swift, so I am not sure what "Swift support" means? In #2079 you mentioned SwiftUI which is a good example, but would be good to document it here.

[brustolin]

I tried this, and it does not work without project refactoring. Not sure if this worth.

[armcknight]

Would this be a breaking change to any current API @brustolin ?

[philipphofmann]

That will be done with #2162

[armcknight]

Refactor our usages of NSDate. This has come up when making some changes for profiling, which uses clock_gettime_nsec_np and falls back to mach_absolute_time for earlier OS versions. Those aren't (easily) compatible with NSDate.

We do have SentryCurrentDate, SentryCurrentDateProvider and SentryDefaultCurrentDateProvider that can return a dispatch_time_t, but almost all of our timekeeping uses NSDate, for span/event/etc start/stop and computing durations. However, NSDate is not a great API for this since it can change at any time, including for time server syncs or when end users change time settings on their devices.

For my current changes I'm just adding a system timestamp alongside the NSDates where needed, but we could refactor this in the future into something like this:

@interface SentryDate : NSObject

/** The @c NSDate reported when this instance was initialized. */
@property (strong, nonatomic) NSDate *nsdate;

/** The system clock time reported when this instance was initialized. */
@property (assign, nonatomic) uint64_t systemTime;
@end

We could also provide methods to calculate durations and compare two dates, and we can check if the calculation differs too greatly between using the NSDate and system time.

I started down this road but wound up having to modify SentryAppStartMeasurement which is a public header.

[armcknight]

So far this comes into play when using startTimestamp/timestamp on spans/events/etc, as well as CFTimeIntervals used in SentryFramesTracker.

[armcknight]

Remove deprecated arrays of enum name constants in favor of functions to map enum values to names (see this PR).

[philipphofmann]

Mark close as deprecated, use close(timeout:) instead, which calls flush. This change aligns with the unified API https://develop.sentry.dev/sdk/unified-api/#client.

[philipphofmann]

Will be done with #1916

[philipphofmann]

@kevinrenskers will take care #2395.

[armcknight]

Closing this as major version 8 has been released. See the new discussion for ideas for next major version 9: #2946