-
Notifications
You must be signed in to change notification settings - Fork 435
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
Fix browser performance issues with configurable maximum variable size #360
Conversation
Hmm Travis seems unhappy for reasons unrelated to this PR... |
…ases wherre inspection causes exceptions.
The Gemfile needs to lock "rake" down to "0.9.6" (or something in that series). Or the Rakefile needs to be updated to work with newer versions of rake. |
lib/better_errors/version.rb
Outdated
@@ -1,3 +1,3 @@ | |||
module BetterErrors | |||
VERSION = "2.1.1" | |||
VERSION = "2.1.1.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you shouldn’t change the VERSION in a pull request. If e.g. v2.2.0 is released before this PR gets merged, you’d need to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, can fix.
OK, have cleaned up the changeset to remove my gem forking stuff, version number and gemspec should be unaffected by this pull request. |
README.md
Outdated
# This will stop BetterErrors from trying to render larger objects, which can cause | ||
# slow loading times and browser performance problems. | ||
# default value: 100_000 | ||
BetterErrors.maximum_variable_inspect_size = 100_000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps specify the unit. This is 100_000 characters, right? Or 100_000 bytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment it's purely bytes character length (CGI.escapeHTML(obj.inspect).length
), it's a very naive approach to get things moving. Happy to look at modifying this if we think there is a better approach, and I'll certainly add this caveat to documentation.
lib/better_errors.rb
Outdated
@@ -139,6 +144,7 @@ def self.default_editor | |||
require "binding_of_caller" | |||
require "better_errors/exception_extension" | |||
BetterErrors.binding_of_caller_available = true | |||
BetterErrors.maximum_variable_inspect_size ||= 100_000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If binding_of_caller
is not available (raises LoadError
), this line won’t be evaluated. I’m not sure where the best location for setting default values would be. Perhaps inside the class definition? Perhaps right after this block?
lib/better_errors/error_page.rb
Outdated
value = CGI.escapeHTML(obj.inspect) | ||
|
||
if !BetterErrors.maximum_variable_inspect_size.nil? && value.length > BetterErrors.maximum_variable_inspect_size | ||
value = "<span class='unsupported'>(object too large - modify #{CGI.escapeHTML(obj.class.to_s)}#inspect or raise BetterErrors.maximum_variable_inspect_size)</span>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of modifying value
here, you could add an else
, e.g.
if length > limit
"oops"
else
value
end
end
html.should_not include "object too large" | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many blank lines here :-)
@@ -88,5 +88,35 @@ def initialize(message = nil) | |||
error_page.exception_message.should_not =~ /\A\n\n/ | |||
end | |||
end | |||
|
|||
context 'with an inspect size limit set' do | |||
before { BetterErrors.maximum_variable_inspect_size = 50_000 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing that you override this limit in the last example, the context
description is a bit misleading. Would be too verbose if we set the limit inside each example?
I have implemented suggested changes (great feedback btw) and retested manually with and without |
Works for me, thank you ! |
Hey @tobypinder, master is green again. Would you mind rebasing on master – and perhaps squashing your commit history a bit? |
@@ -143,4 +148,6 @@ def self.default_editor | |||
BetterErrors.binding_of_caller_available = false | |||
end | |||
|
|||
BetterErrors.maximum_variable_inspect_size ||= 100_000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m wondering if setting the default value should be moved to line 53, right after setting the default for ignored_instance_variables
?
I realize that the value of binding_of_caller_available
is being set down here, but that is probably just to avoid having require
s inside the module code.
it "hides variables with inspects that are above the inspect size threshold" do | ||
BetterErrors.maximum_variable_inspect_size = 50_000 | ||
|
||
content = 'A' * (BetterErrors.maximum_variable_inspect_size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parentheses are not necessary here.
it "shows variables with large inspects if max inspect size is disabled" do | ||
BetterErrors.maximum_variable_inspect_size = nil | ||
|
||
content = 'A' * (50_000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parentheses are not necessary here.
it "shows variables with large inspects if max inspect size is disabled" do | ||
BetterErrors.maximum_variable_inspect_size = nil | ||
|
||
content = 'A' * (50_000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be nice to test if this is over default (so default is overridden by nil
).
The project has been converted to RSpec 3's "expectation" syntax, so the specs need a slight rewording. If you're not familiar with the new syntax, you can use transpec to perform the conversion for you. |
@RobinDaugherty Happy to take a look at these within the next day or so. |
I changed the RSpec syntax and addressed the other PR comments as well – and rebased on master. Pushed the changes to a branch on my fork: master...bquorning:quieter_output Let me know if I should open a new pull request. |
@bquorning Looks great, sorry I couldn't get around to it! |
@tobypinder Would you update this PR with my commits, or should I open a new pull request? |
@bquorning probably better for you to open a new PR if that's easier? Happy to close this one. |
It’s at #402. This one should automatically be closes when 402 is merged. |
Based on @bquorning's comment on #334, this will allow the user to set a maximum payload size for
variable.inspect
, above which the variable is filtered out of the error page. This solves many performance problems associated with the browser being slow to render error pages, the CPU requirements for the error page skyrocketing etc.Many variable types end up being huge like Rails collections being inspected (and therefore queried for) before they are paginated. I've set the default to 100,000 characters, but the default can be modified to unlimited by setting
BetterErrors.maximum_variable_inspect_size
tonil
, if we think compatibility is more important than a friendly default.Keen to work to get this PR merged, there should be test coverage and documentation but feedback is appreciated!