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 project_root prefix from filename #278

Merged
merged 1 commit into from
Jan 22, 2015

Conversation

eagletmt
Copy link
Contributor

@eagletmt eagletmt commented Jan 7, 2015

Especially in Rails project, when an exception is raised in view template, the filename is always full path.
With capistrano (de-facto standard for Ruby application deployment), the full path differs on each release.
It causes Sentry to recognize the same error differently.

@seanlinsley
Copy link
Contributor

Hmm, my Rails application uses Capistrano and doesn't have an issue with Sentry failing to group events.

@eagletmt
Copy link
Contributor Author

eagletmt commented Jan 8, 2015

Hmm... my Rails application always says the filename is /path/to/deploy/directory/releases/$TIMESTAMP/app/views/foo/bar.html.erb .

Even if the filename was /path/to/deploy/directory/current, truncating meaningless part would be better.

@seanlinsley
Copy link
Contributor

What version of Rails and Capistrano are you using?

@eagletmt
Copy link
Contributor Author

eagletmt commented Jan 8, 2015

@seanlinsley
Copy link
Contributor

I'm using Rails 4.1 with Capistrano 2.15.5

@eagletmt
Copy link
Contributor Author

eagletmt commented Jan 8, 2015

Again, even if the filename was /path/to/deploy/directory/current/app/views/foo/bar.html.erb , truncating meaningless part would be better.
app/views/foo/bar.html.erb is a familiar form to Rails users.

Model's or controller's filename is book.rb or books_controller.rb in Rails project.
Only view templates have full path currently.

@nateberkopec
Copy link
Contributor

@dcramer Should Sentry be able to correctly group the case described here?

@nateberkopec
Copy link
Contributor

Sorry, but I think we have to keep abs_path as the actual absolute path to stay on spec. If you think there's an issue with how Sentry groups events together, please open that issue on the main Sentry server repository.

@nateberkopec
Copy link
Contributor

Hmm not sure I quite understood this PR, you weren't actually changing abs_path, you were changing filename!

@nateberkopec nateberkopec reopened this Jan 21, 2015
@dannyfallon
Copy link

👍 @eagletmt, this is precisely what I was getting at in the issue

@@ -36,6 +36,12 @@ def filename
return nil if self.abs_path.nil?

prefix = $LOAD_PATH.select { |s| self.abs_path.start_with?(s.to_s) }.sort_by { |s| s.to_s.length }.last
if prefix.nil? && Raven.configuration.project_root
project_root = Raven.configuration.project_root.to_s
if self.abs_path.start_with?(project_root)
Copy link
Contributor

Choose a reason for hiding this comment

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

This self is unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confusing I know since we use it everywhere else in this method.

@nateberkopec
Copy link
Contributor

@eagletmt If you could please rebase against master, I can merge.

@eagletmt
Copy link
Contributor Author

@nateberkopec rebased.

nateberkopec added a commit that referenced this pull request Jan 22, 2015
Truncate project_root prefix from filename
@nateberkopec nateberkopec merged commit 6ed265c into getsentry:master Jan 22, 2015
@eagletmt eagletmt deleted the relative-path branch January 22, 2015 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants