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

Stracktrace frames should strip the project_dir from abs_path for filenames #288

Closed
dannyfallon opened this issue Jan 17, 2015 · 8 comments

Comments

@dannyfallon
Copy link

In the filename method you're currently using the $LOAD_PATH array to determine if you can strip the start of the path for the current stacktrace frame. In Rails applications this presents some disadvantages, all related to grouping errors in Sentry

By not including the Rails root or project_dir when attempting to strip prefixes from the absolute path you end up using the full path for errors in places such as views, initialisers etc. This presents a problem if your path contains a variable, for example the timestamp or the SHA for the current version. You essentially end up polluting your sentry list with errors that should be grouped together. Right now we're monkeypatching the filename method to work around this issue

In a bit of a tangent, perhaps using the full $LOAD_PATH is also causing some unintended problems for Rails apps. The Rails autoloader will add some paths such as /app/models, /app/controllers and /lib by default. Imagine a scenario where you've a model named conversation.rb and you've a lib of the same name. The filename for an error that happens in either will be the same, so the culprit will be the same. This presents issues in the Sentry UI listing, both errors have the same culprit (conversation.rb). It also makes things difficult when viewing the backtrace, which doesn't appear to expose the abs_path in an easy way.

@nateberkopec
Copy link
Contributor

Related: #278

@dannyfallon
Copy link
Author

Cheers Nate. We've been running the monkeypatch in production the last few days and it has certainly cleared up the grouping of errors. I'll try submit a PR this week for that behaviour for review

@nateberkopec
Copy link
Contributor

@dcramer Can you chime in on what parts of this abs_path issue are something we should solve in the client vs on the Sentry server?

@dcramer
Copy link
Member

dcramer commented Jan 21, 2015

Just to copy convo from IRC:

  • absolute path is designed to be the absolute path on disk
  • filename is designed to be some kind of normalized or relative path (i.e. you can trim project root)

@dannyfallon
Copy link
Author

Thanks @dcramer, #278 looks to solve the first issue where the Rails root remains on the filename.

How about your thoughts on the second one? Rails' autoloader adds some directories to the $LOAD_PATH, it might confuse some users by removing valuable path information

@dcramer
Copy link
Member

dcramer commented Jan 21, 2015

Assuming $LOAD_PATH is the same as PYTHONPATH, I think it's fine to only store the full path in abspath and truncate at $LOAD_PATH, but I'm not really sure here.

@nateberkopec
Copy link
Contributor

@dannyfallon Is there any way we could produce a "failing test" for your second point?

@nateberkopec
Copy link
Contributor

Solved by #278 and #291

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

No branches or pull requests

3 participants