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.

Sign up for GitHub

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

Jump to bottom

[rails] fix non-finishing span problem #48

Merged
merged 2 commits into from
Jan 3, 2017
Merged

[rails] fix non-finishing span problem #48

merged 2 commits into from
Jan 3, 2017

Conversation

ufoot
Copy link
Contributor

@ufoot ufoot commented Dec 30, 2016

This PR does two things:

  • address a possible issue on process_action.action_controller, when :status is not defined
  • in a more general manner, try and finish spans whatever happens. Still log errors, but at least flush and send spans once they've been created.

@ufoot
[rails] returning unknown status when :status is not defined
b9e168f 
When calling process_action.action_controller, :status should be
defined (http://guides.rubyonrails.org/active_support_instrumentation.html)
but sometimes it's not. In that case return '?' rather than firing
an error and not sending the trace at all.
@ufoot
[rails] ensure spans are always finished
9fd960c 
We already have a handler to catch errors in rails intrumentation,
but on error, spans might simply not be finished and sent. This
patch ensures a best effort to finish spans once they have been
created.
@ufoot ufoot requested a review from palazzem December 30, 2016 10:34
@ufoot ufoot added the bug Involves a bug label Dec 30, 2016
@palazzem palazzem self-assigned this Dec 30, 2016
palazzem
palazzem reviewed Dec 30, 2016
View reviewed changes
Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

I think the code is fine and it should also handle unexpected behaviors. Anyway will try it later before merging to master.

palazzem
palazzem approved these changes Jan 3, 2017
View reviewed changes
Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

LGTM!

@ufoot ufoot merged commit bcb7de0 into master Jan 3, 2017
@palazzem palazzem deleted the christian/railsfailstatus branch January 11, 2017 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@palazzem palazzem palazzem approved these changes

Assignees

@palazzem palazzem

Labels
bug Involves a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ufoot @palazzem