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

Truncate the message sent to sentry to max allowable chars #806

Merged
merged 3 commits into from
Apr 27, 2020

Conversation

max-b
Copy link

@max-b max-b commented Apr 27, 2020

This attempts to address the issue described in https://github.com/getlantern/lantern-internal/issues/3620#issuecomment-616904350 where panics in the anacrolix/torrent library weren't getting reported to sentry.

From some random testing locally, it seems as though the stack trace we're attempting to sent to sentry is just too large and is causing the reporting to fail silently 😬 😬 (I think this is "documented" in getsentry/sentry-go#169)

I chose 8k as the max number of characters from basically looking at successful sentry reports and looking at the point at which they were truncated (I think that sentry does some kind of truncation before storing on their end).

I was able to get a panic inside a goroutine in the torrent library to be reported after making the change:
https://sentry.io/organizations/getlantern/issues/1633600374/?project=2222244

desktop/app.go Outdated Show resolved Hide resolved
@max-b max-b requested a review from myleshorton April 27, 2020 17:14
@myleshorton myleshorton self-assigned this Apr 27, 2020
@max-b
Copy link
Author

max-b commented Apr 27, 2020

Oh I started investigating that issue on the sentry-go sdk repo and I think they may have actually just implemented this in v0.6.0.

Lemme double check that's the case and they we can just bump their version instead of this handrolled implementation...

@max-b
Copy link
Author

max-b commented Apr 27, 2020

Oh I started investigating that issue on the sentry-go sdk repo and I think they may have actually just implemented this in v0.6.0.

Lemme double check that's the case and they we can just bump their version instead of this handrolled implementation...

hmmm nope as far as I can tell it's still silently failing :/ I think if I'm reading the getsentry/sentry-go#168 correctly, it is just ensuring that the http integrations don't send too large of a body to the sentry client. Since we're manually sending the message I think we have to do that ourselves...

@myleshorton
Copy link
Contributor

LGTM!

@max-b
Copy link
Author

max-b commented Apr 27, 2020

I'm gonna do a quick test of moving this into the BeforeSend function in the sentry options just to tidy it up a little and to make sure that we don't run into this again if we end up needing to use CaptureMessage somewhere else in the codebase..

@max-b
Copy link
Author

max-b commented Apr 27, 2020

Ok yeah I did a smol refactor and I like this way more. I tested that it still reports correctly: https://sentry.io/organizations/getlantern/issues/1633698328/?project=2222244&query=is%3Aunresolved

Thanks for being patient with me 😁 😁

@myleshorton
Copy link
Contributor

Looks great @max-b -- gonna go ahead and merge!

@myleshorton myleshorton merged commit bb6bb66 into devel Apr 27, 2020
@myleshorton myleshorton deleted the maxb/sentry-reporting-fix branch April 27, 2020 22:05
@max-b
Copy link
Author

max-b commented Apr 27, 2020

Thanks!

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