Skip to content

Conversation

@buenaflor
Copy link
Contributor

@buenaflor buenaflor commented Mar 31, 2023

ref: #66

@buenaflor buenaflor changed the title fix: cocoa crash handling due to sdkInfo removal in cocoa sdk #67 fix: cocoa crash handling due to sdkInfo removal in cocoa sdk Mar 31, 2023

event?.sdk = sdk
event
dropKotlinCrashEvent(event as NSExceptionSentryEvent?) as SentryEvent?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since beforeSend is based on the lambda result we have to put dropKotlinCrashEvent at the end.

@buenaflor buenaflor marked this pull request as ready for review April 3, 2023 07:46
@buenaflor buenaflor requested a review from marandaneto April 3, 2023 07:46
val item = SentryEnvelopeItem(preparedEvent)
// TODO: pass traceState when enabling performance monitoring for KMP SDK
val header = SentryEnvelopeHeader(preparedEvent.eventId, SentrySDK.options?.sdkInfo, null)
val header = SentryEnvelopeHeader(preparedEvent.eventId, null)
Copy link
Contributor

Choose a reason for hiding this comment

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

M: why is the sdkInfo removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does that mean that events dont have the SDK info anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's removed because the cocoa sdk deprecated sdkInfo. So SentryEnvelopeHeader does not have a param in the constructor with sdkInfo anymore.

The sdkInfo is still being set in beforeSend with PrivateSentrySDKOnly.setSdkName(sdkName, sdkVersion)

val causes = mutableSetOf<Throwable>()
var cause = cause
while (cause != null && cause !in causes) {
while (cause != null && causes.add(cause)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just an optimization? can causes.add return false for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, just a minor optimization - add(...) will return false on elements that are already in a set.

@buenaflor buenaflor merged commit d5ecbdf into main Apr 6, 2023
@buenaflor buenaflor deleted the fix/cocoa-crash-handling branch April 6, 2023 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants