Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error is associated with incorrect transaction after crash #4375

Closed
aellett opened this issue Sep 26, 2024 · 4 comments · Fixed by #4393
Closed

Error is associated with incorrect transaction after crash #4375

aellett opened this issue Sep 26, 2024 · 4 comments · Fixed by #4393

Comments

@aellett
Copy link

aellett commented Sep 26, 2024

Platform

iOS

Environment

Production

Installed

Manually

Version

8.36.0

Xcode Version

15.2

Did it work on previous versions?

No response

Steps to Reproduce

  1. Start a scoped transaction (not sure if this step is necessary, but I had one running in my project).
  2. Cause a fatal crash (I used index out of bounds).
  3. After the app crashes, launch the app again.
  4. Start a new scoped transaction in AppDelegate.init() (as soon as possible after Sentry is configured). In my screenshots, this transaction is called LAUNCH_[obscured]
  5. Find the fatal crash in the Sentry console.

Expected Result

I would expect to see either

The error has a transaction property that matches the name of the original scoped transaction (before the crash)

or

The error has a transaction property that is nil, or doesn't have a transaction property at all.

Actual Result

The error is labeled with transaction of LAUNCH_[obscured] which was the transaction that I started after the second app launch.

I think that I would rather not see a transaction at all, if the alternative is to see the next transaction after the crash. This is further misleading (for me) because it shows up in the title at the top of the screen on the view of the error (screenshot attached). If I ever wanted to count how many errors had some particular transaction value, I think this would throw off my calculations.

Note: I imagine this is sort of adjacent to #2306 so if it's more convenient to group these together, feel free, but this felt like a separate issue. Also felt like it might be able to be resolved even if 2306 can't be resolved.

Are you willing to submit a PR?

No response

@aellett
Copy link
Author

aellett commented Sep 26, 2024

Oops, forgot to attache the screenshots I mentioned.

Image
Image

@philipphofmann
Copy link
Member

philipphofmann commented Sep 27, 2024

Thanks for reporting this, @aellett. What exactly do you mean by

Start a scoped transaction (not sure if this step is necessary, but I had one running in my project).

I tried to reproduce the issue but I couldn't. I start a manual transaction directly after SentrySDK.start like this:

let t = SentrySDK.startTransaction(name: "after-crash", operation: "crash", bindToScope: true)
let child = t.startChild(operation: "hello")

DispatchQueue.global().asyncAfter(deadline: .now() + 10.0) {
    child.finish()
    t.finish()
}

Potential culprits could be that we have to ignore crash events here

if (self.span != nil) {
id<SentrySpan> span;
@synchronized(_spanLock) {
span = self.span;
}
// Span could be nil as we do the first check outside the synchronize
if (span != nil) {
if (![event.type isEqualToString:SentryEnvelopeItemTypeTransaction] &&
[span isKindOfClass:[SentryTracer class]]) {
event.transaction = [[(SentryTracer *)span transactionContext] name];
}
newContext[@"trace"] = [span serialize];
}
}
if (newContext[@"trace"] == nil) {
// If this is an error event we need to add the distributed trace context.
newContext[@"trace"] = [self.propagationContext traceContextForEvent];
}

and here

- (nullable SentryTraceContext *)getTraceStateWithEvent:(SentryEvent *)event
withScope:(SentryScope *)scope
{
id<SentrySpan> span;
if ([event isKindOfClass:[SentryTransaction class]]) {
span = [(SentryTransaction *)event trace];
} else {
// Even envelopes without transactions can contain the trace state, allowing Sentry to
// eventually sample attachments belonging to a transaction.
span = scope.span;
}
SentryTracer *tracer = [SentryTracer getTracer:span];
if (tracer != nil) {
return [[SentryTraceContext alloc] initWithTracer:tracer scope:scope options:_options];
}
if (event.error || event.exceptions.count > 0) {
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
return [[SentryTraceContext alloc] initWithTraceId:scope.propagationContext.traceId
options:self.options
userSegment:scope.userObject.segment
replayId:scope.replayId];
#pragma clang diagnostic pop
}
return nil;
}

@philipphofmann philipphofmann moved this from Needs Discussion to Needs More Information in Mobile & Cross Platform SDK Sep 27, 2024
@aellett
Copy link
Author

aellett commented Sep 30, 2024

Thanks for reporting this, @aellett. What exactly do you mean by

Start a scoped transaction (not sure if this step is necessary, but I had one running in my project).

I tried to reproduce the issue but I couldn't. I start a manual transaction directly after SentrySDK.start like this:

I just meant that I wouldn't think that having a transaction running when the crash happens matters.

If I cause an error, but the transaction field of the error refers to a transaction (LAUNCH_ in my case) that hasn't started yet, that seems wrong to me.

It looks to me like you're correct that if those places (maybe just the first one?) ignored crash events, it would resolve this.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Sep 30, 2024
@philipphofmann philipphofmann self-assigned this Oct 2, 2024
@philipphofmann philipphofmann moved this from Needs More Information to Todo in Mobile & Cross Platform SDK Oct 2, 2024
@getsantry getsantry bot removed the status in GitHub Issues with 👀 3 Oct 2, 2024
@philipphofmann philipphofmann moved this from Todo to In Progress in Mobile & Cross Platform SDK Oct 3, 2024
philipphofmann added a commit that referenced this issue Oct 3, 2024
Fix linking a crash event to an ongoing trace by skipping setting the
trace context in Scope.applyToEvent for crash events.

Fixes GH-4375
@philipphofmann philipphofmann moved this from In Progress to Needs Review in Mobile & Cross Platform SDK Oct 3, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in Mobile & Cross Platform SDK Oct 3, 2024
@philipphofmann
Copy link
Member

@aellett, we fixed the issue. We plan to release this fix early next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants