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

Fixed "Culprit" recording in Sentry #14

Merged
merged 4 commits into from
Apr 26, 2018
Merged

Conversation

slightfoot
Copy link
Contributor

Fix stack-trace frame ordering. In order for Culprit to be logged correctly it should have the latest frame at the top.

@slightfoot
Copy link
Contributor Author

@yjbanov I run the Dart tests locally and they all succeed with Dart 2. The Travis error looks to be related to typing and Dart 2. Perhaps you can take a look.

@yjbanov
Copy link
Contributor

yjbanov commented Apr 16, 2018

Thanks for the fix! Are there any docs that describe how culprit works? I'm also wondering if populating it explicitly here is a more robust way to go?

@slightfoot
Copy link
Contributor Author

slightfoot commented Apr 16, 2018

It's more about how the Sentry site displays the culprit. It's wrong without the stack in reverse. This is probably more to do with the fact that Sentry doesn't 'officially' support Dart. In which case they might automatically reverse it.

This snippet will show you my implementation.
https://gist.github.com/slightfoot/094657bb22e986bbb4c9bafd9841cbd8

@yjbanov
Copy link
Contributor

yjbanov commented Apr 26, 2018

Ok, looks like the only way to know for sure is to try it. I can't think of anything that would break from this change other than surprise some people when they see the stacks reversed. So let's give it a shot. Let me look into the type errors first.

@slightfoot
Copy link
Contributor Author

@yjbanov once this is merged and the package released. I can push out my crash reporting solution.

@yjbanov
Copy link
Contributor

yjbanov commented Apr 26, 2018

FYI #15 hopefully fixes the type issue 🤞

@yjbanov
Copy link
Contributor

yjbanov commented Apr 26, 2018

Could you please rebase this PR on top of tip of tree? If it's green on Travis let's merge it.

@slightfoot
Copy link
Contributor Author

@yjbanov rebased, any ideas on the travis issue?

@yjbanov
Copy link
Contributor

yjbanov commented Apr 26, 2018

Hmm, it's as if part of the log file is missing. I restarted the job in case it was a flake.

@yjbanov
Copy link
Contributor

yjbanov commented Apr 26, 2018

Oh wait, you just need to dartfmt -w test/sentry_test.dart

@yjbanov
Copy link
Contributor

yjbanov commented Apr 26, 2018

Thanks for fixing this! Merging. I'll do a release shortly.

@yjbanov yjbanov merged commit f611b80 into getsentry:master Apr 26, 2018
@slightfoot
Copy link
Contributor Author

Thanks @yjbanov.

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.

2 participants