Feature #18438
closedAdd `Exception#additional_message` to show additional error information
Description
Proposal¶
I'd like to introduce a method Exception#additional_message
, and let the Ruby error printer show it after Exception#message
.
class MyError < StandardError
def message = "my error!"
def additional_message = "This is\nan additional\nmessage"
end
raise MyError
$ ./miniruby test.rb
test.rb:6:in `<main>': my error! (MyError)
| This is
| an additional
| message
PoC implementation: https://github.com/ruby/ruby/pull/5351
Motivation¶
At the present time, did_you_mean and error_highlight overrides Exception#message
to add their suggestions.
begin; 1.time; rescue NoMethodError; pp $!.message; end
#=> "undefined method `time' for 1:Integer\n" +
# "\n" +
# " 1.time\n" +
# " ^^^^^\n" +
# "Did you mean? times"
This implementation approach has a practical problem. Because it changes the return value of Exception#message
, it breaks a test that checks the return value of Exception#message
.
Though such a test is never recommended, I encountered some actual cases when creating error_highlight. See the change of tests of minitest as a typical example: https://github.com/seattlerb/minitest/pull/880/files
Currently, error_highlight shows hint information only for NoMethodError because it is relatively rare to check the message of NameError
. Still, it broke some tests unfortunately, though. If possible, I'd like to add suggestions to other kinds of errors, but it will break much more tests.
If Exception#additional_message
is introduced, and if did_you_mean and error_highlight overrides the method to add their suggestions, this problem will not occur because they no longer changes the result value of #message
method.
Cooperation needed¶
Currently, many Ruby/Rails users montiors their production services by using application monitoring services such as Sentry, DataDog, ScoutAPM, etc. The original motivation of error_highlight is for production (see #17930), so it will lose the significance if such services do not support Exception#additional_message
. So, I'd like to hear opinions from developers of such services. If they are against this proposal or if we can't get their cooperation, I don't think my proposal should be accepted.
If you are a developer of these services, I would be very grateful if you could comment on this ticket. @ivoanjo (Ivo Anjo)
Bikesheds¶
- I'm unsure if
Exception#additional_message
is a good name. Please propose alternatives if it is not good. - Currently, the result of
addtional_message
is printed with no escape. This may be a more compatible solution against https://bugs.ruby-lang.org/issues/18367. - It may be good to let
Exception#additional_message
accepthighlight
keyword as boolean information whether the output target is a terminal or not. CurrentlyException#full_message
accepts it. I have no plan to use the information inerror_highlight
, though. Not onlyhighlight
but also any keywords may be forwarded fromfull_message(**opt)
toadditional_message(**opt)
for future use case. - My current PoC adds prefixs "
|
" before each line ofaddtional_message
. I'm unsure if this is good or bad. I'm happy to change or remove the prefixes.
Files
Updated by st0012 (Stan Lo) almost 3 years ago
Hello, I'm Stan, the maintainer of Sentry's Ruby SDK.
I think Sentry currently displays the overridden exception message as intended (see the image below). So to us, this separation is not needed atm.
But if you want to extract the message as additional_message
to keep things clean, that'll work for us too. Since Sentry doesn't accept any complementary attribute for exceptions, I'll just concat it with message
and it should look the same.
Thanks for implementing such a great feature :-)
Updated by mame (Yusuke Endoh) almost 3 years ago
@st0012 (Stan Lo) Thank you very much for your reply!
So to us, this separation is not needed atm.
The motivation of this separation is not for your services. Some Ruby programmers write a test to check the exception message like this.
exc = (raise SomeError rescue $!)
assert_equal("exception message", exc.message)
This test will break if error_highlight extends SomeError
class because currently error_highlight changes the return value of SomeError#message
. If error_highlight overrides #additional_message
, this kind of failures will not occur.
But if you want to extract the message as additional_message to keep things clean, that'll work for us too. Since Sentry doesn't accept any complementary attribute for exceptions, I'll just concat it with message and it should look the same.
Yes, I think that's a good enough way to handle the change!
Updated by ivoanjo (Ivo Anjo) almost 3 years ago
Thanks for the ping!
I'll need to sync with my colleagues at Datadog and may take a few days because a lot of people are off, but I have a few questions:
- Would
additional_message
only be something that gems like did_you_mean and error_highlight use (e.g. better debugging/developer experience tools), or do you see normal user code using it as well? - The prefixed
|
seem harmless, but could you clarify what their use-case is? Is it to make it clear, when printing, which part is the regular message and which part is the additional one?
Updated by byroot (Jean Boussier) almost 3 years ago
What if instead of having two distinct #message
and #additional_message
that need to be concatenated, there would simply be an alternative, more descriptive method?
e.g.
Exception#description
which would be:
def message
@message
end
def description(highlight: false)
"#{message}\nSome extra info"
end
def full_message(highlight: false, order: ...)
"#{description(highlight: highlight)}\n#{backtrace...}"
end
The advantage being that it allows description
to modify message
.
Updated by mame (Yusuke Endoh) almost 3 years ago
ivoanjo (Ivo Anjo) wrote in #note-3:
- Would
additional_message
only be something that gems like did_you_mean and error_highlight use (e.g. better debugging/developer experience tools), or do you see normal user code using it as well?
My current motivation is only did_you_mean and error_highlight. But it is difficult to force everyone else not to use it, so I guess some users will use it for any reason that I don't see currently.
- The prefixed
|
seem harmless, but could you clarify what their use-case is? Is it to make it clear, when printing, which part is the regular message and which part is the additional one?
I have no strong use case. I'm okay to remove the prefix. I thought that they were useful to distinguish between a regular message and additional one, but it is possible to "fake" it by crafting a #message
, such as "the first line\n| faked additional message"
.
byroot (Jean Boussier) wrote in #note-4:
What if instead of having two distinct
#message
and#additional_message
that need to be concatenated, there would simply be an alternative, more descriptive method?
snip
The advantage being that it allowsdescription
to modifymessage
.
I wonder if it is a good idea or not. Allowing to modify message
might be useful in some cases, but I'm afraid if it would bring rather confusion.
Updated by Dan0042 (Daniel DeLorme) almost 3 years ago
So this serves roughly the same purpose as #18296
I also have a slight preference for a description
method that includes the message
. It seem simpler for everyone (including Sentry, Datadog, etc.) to use e.description
instead of e.message + "\n" + e.additional_message
, and also easier to support older rubies. But I'm curious if the additional_message
approach has some advantage I overlooked.
There's also the question of who/what is responsible for the ansi/terminal formatting? It seems in this design, additional_message
is a simple string and all formatting (bold) is handled by full_message
. Nice and simple. With description
we can (should?) delegate all formatting to this method, so it's very flexible and powerful.
byroot (Jean Boussier) wrote in #note-4:
Exception#description
which would be:
I had the same thought initially but there's a hiccup: the description needed by full_message should include the (underlined) error type, but the description needed by Sentry should not. That makes the design somewhat less elegant. You would need something like
class Exception
def description(highlight: false, append_type: false, **)
msg = message
if append_type
type = self.class.name
type = ANSI_UNDERLINE_ON + type + ANSI_UNDERLINE_OFF if highlight
msg = msg.sub(/$/, " (" + type + ")")
end
msg = ANSI_BOLD_ON + msg + ANSI_BOLD_OFF if highlight
msg
end
def full_message(**opts)
description(highlight: true, append_type: true, **opts) + "\n" + backtrace_str
end
end
Updated by Eregon (Benoit Daloze) almost 3 years ago
- Related to Feature #18296: Custom exception formatting should override `Exception#full_message`. added
Updated by Eregon (Benoit Daloze) almost 3 years ago
@mame (Yusuke Endoh) Thanks for making this proposal, I believe it goes in the right direction.
Dan0042 (Daniel DeLorme) wrote in #note-6:
I had the same thought initially but there's a hiccup: the description needed by full_message should include the (underlined) error type, but the description needed by Sentry should not. That makes the design somewhat less elegant. You would need something like
That part, showing the exception class name, should be handled by #full_message
, like it currently is.
description
would only return "a more complete message" but not try to do any formatting which is already handled by #full_message
(like the exception class, backtrace, causes, etc).
In fact, full_message
is far more complicated than what is shown in @byroot's example (which is fine since it's illustrative).
See https://github.com/oracle/truffleruby/blob/39d740bd3eb8abafe737656055850fc7b3a4d1dc/src/main/ruby/truffleruby/core/exception.rb#L90-L130 and https://github.com/oracle/truffleruby/blob/39d740bd3eb8abafe737656055850fc7b3a4d1dc/src/main/ruby/truffleruby/core/truffle/exception_operations.rb#L94-L112 for example.
I think the description
approach is nice because it's easier to call (no manual concat) and also more flexible (access to original message, access to kwargs). +1 from me.
Monitoring services can then just call description(highlight: false)
instead of message
.
Updated by marcotc (Marco Costa) almost 3 years ago
👋, I maintain Datadog's application monitoring gem, alongside Ivo.
ivoanjo (Ivo Anjo) wrote in #note-3:
I'll need to sync with my colleagues at Datadog and may take a few days because a lot of people are off
Following up here, I agree with the description
proposal:
Eregon (Benoit Daloze) wrote in #note-8:
I think the
description
approach is nice because it's easier to call (no manual concat) and also more flexible (access to original message, access to kwargs). +1 from me.
Monitoring services can then just calldescription(highlight: false)
instead ofmessage
.
For us, calling description(highlight: false)
to have the richest possible textual information, then separately calling backtrace
to get the code location would be the ideal APIs for error reporting.
Updated by nobu (Nobuyoshi Nakada) almost 3 years ago
Exception#description
seems to add too much flexibility and make responsibilities vague.
Updated by Eregon (Benoit Daloze) almost 3 years ago
@nobu (Nobuyoshi Nakada) How so? Do you have an example?
The responsibilities for description
are to call super
and include it in the return value. That's it, nothing else.
The same goes for additional_message
and is necessary if there are multiple definitions of additional_message
in the ancestors.
Except it's less clear for additional_message
because the default additional_message
returns ""
.
And so how to avoid an extra \n
before? That'd probably means harder to write a correct additional_message
=> msg = super; msg.empty? ? extra : "#{msg}\n#{extra}"
.
This is not a problem for description
, where super
always returns a non-empty String (unless the original message is empty, but that's then basically a bug of who raised it) => "#{super}\n#{extra}
.
The class, backtraces and causes are added later by full_message
, because there is no value to add them before and it's not trivial to add them (the class is always shown on the first line, even for a multi-line message, that's already current behavior).
Updated by Dan0042 (Daniel DeLorme) almost 3 years ago
@nobu (Nobuyoshi Nakada) did you mean that the name "description" is vague or the concept itself? Would you find it less vague if it was named something else? (e.g. "detailed_message" or something)
Updated by st0012 (Stan Lo) almost 3 years ago
It seem simpler for everyone (including Sentry, Datadog, etc.) to use e.description instead of e.message + "\n" + e.additional_message, and also easier to support older rubies.
Not saying I like that but I think it's generally ok because
- Not many developers need to do this. Perhaps just SDK authors and some gem authors.
- We still need to make the change anyway and maintain compatibility between versions.
Exception#description seems to add too much flexibility and make responsibilities vague.
I also think description
could be confusing. I'd say it's mainly because of the name instead of the concept. If I understand it correctly, it looks like full_message
will be built on top of description
, which will be message
+ additional information (like from error_lighlight
)?
To me, it's hard to tell the difference between description
, message
and full_message
from their names and certainly doesn't help understand the relationship between them.
I think complemented_message
will be a clearer (but also verbose) name?
Then we'll have:
-
message
- the most fundamental message of the exception. -
complemented_message
- message with additional information to help debugging -
full_message
- all of the above + type info + backtrace...etc.
Updated by mame (Yusuke Endoh) almost 3 years ago
We discussed this topic yesterday at the dev-meeting. @matz (Yukihiro Matsumoto) basically liked the API style of Exception#description
, but disliked the name. full_message
calls description
, and description
calls message
, which looked awkward to matz. Anyone have a good suggestion about alternative names? Maybe *_message
which comes between full_message
and just `message would be preferable.
Updated by Eregon (Benoit Daloze) almost 3 years ago
detailed_message
maybe? (also the term used in https://bugs.ruby-lang.org/issues/18296#note-20)
IMHO description
is fine and a good term in relation to message
.
full_message
is a weird name to me because it includes the backtrace and it's not really a "message" anymore, but that's how it is and seems unlikely worth to be renamed now that it is established.
Updated by mame (Yusuke Endoh) almost 3 years ago
@matz (Yukihiro Matsumoto) accepted the name detailed_message
. I'll update and improve my PR (or volunteer is welcome)
Note: the rdoc of detailed_message
should explicitly state that the method should accept highlight
keyword that allows using ANSI terminal escape sequences to highlight the error message. Also, the method should accept and ignore any unknown keywords because some gems (say, error_highlight) can accept a custom keyword to enable their own formatting customization (say, error.full_message(error_highlight: false)
.
Updated by Dan0042 (Daniel DeLorme) almost 3 years ago
Very happy about this.
mame (Yusuke Endoh) wrote in #note-16:
Note: the rdoc of
detailed_message
should explicitly state that the method should accepthighlight
keyword that allows using ANSI terminal escape sequences to highlight the error message.
I'd like to ask for clarification regarding this. In #full_message, the #detailed_message is wrapped with bold, correct? In that case the highlight
keyword for #detailed_message is for any highlighting except bold? That would also mean #detailed_message must not use "\e[0m" otherwise that would break the highlighting of #full_message? I think it would be good to clarify how the responsability of terminal highlighting is shared between #full_message and #detailed_message.
Updated by mame (Yusuke Endoh) almost 3 years ago
I talked with @matz (Yukihiro Matsumoto) and @nobu (Nobuyoshi Nakada) about that.
The current implementation of Exception#full_message
adds \e[1m
(i.e., bold start) for each beginning of line of #message
, and e[m
(i.e., bold end) for each end. (Also, it adds " (RuntimeError)"
or something to the last of the first line.)
With that in mind, #detailed_message
can freely add their own escape sequences. Exception#full_message
won't check the result of detailed_message
.
BTW, the behavior of Exception#full_message
(adding \e[1m
and e[m
) will be moved to Exception#detailed_message
. That is, a rough spec will be like the following.
class Exception
def detailed_message
lines = message.lines.map { "\e[1m" + _1.chomp + "\e[m" }
lines[0] << " (#{ self.class.inspect })"
lines.join("\n")
end
def full_message
# frame is a string like "file.rb:lineno:in `method_name'"
head_frame, *tail_frames = backtrace
"#{ head_frame }: #{ detailed_message }\n" + tail_frames.map { "\tfrom " + _1 }.join("\n")
end
end
class MyError < StandardError
def detailed_message
# In general, user-defined detailed_message should prepend the result of "super"
"#{ super }\naddtional message"
end
end
Note that the "addtional message" part of MyError#detailed_message
will not be automatically highlighted.
Updated by Dan0042 (Daniel DeLorme) almost 3 years ago
Thank you, it's much clearer now.
Follow-up question: Should #detailed_message include the class of the error
a) always (unlike #message currently)
b) only if highlight
keyword is true
c) based on some other keyword (like in #note-6)
?
nitpick: "\e[0m" or "\e[m" is for full reset; imho it would be better to use "\e[22m" for bold-off (and "\e[24m" for underline-off). It allows things like adding color: "\e[31m" + err.detailed_message(highlight:true) + "\e[39m"
.
Updated by mame (Yusuke Endoh) almost 3 years ago
I have created another ticket to summarize the final spec of this proposal: #18564.
Updated by mame (Yusuke Endoh) almost 3 years ago
@Dan0042 (Daniel DeLorme) I think your last comment is a different topic. I'd like to keep one proposal as small as I can.
Updated by Dan0042 (Daniel DeLorme) almost 3 years ago
My concern was that Sentry will now use #detailed_message instead of #message, but it will now include the exception class, unlike before. But if Sentry is ok with that there's no problem.
Updated by mame (Yusuke Endoh) almost 3 years ago
Dan0042 (Daniel DeLorme) wrote in #note-22:
My concern was that Sentry will now use #detailed_message instead of #message, but it will now include the exception class, unlike before. But if Sentry is ok with that there's no problem.
Ah, that's a good point. @matz (Yukihiro Matsumoto) what do you think?
Updated by st0012 (Stan Lo) almost 3 years ago
My concern was that Sentry will now use #detailed_message instead of #message, but it will now include the exception class, unlike before. But if Sentry is ok with that there's no problem.
I think it'll be fine but I'm not sure why the class name needs to be added as well?
And just for confirmation: so if I want to preserve the content of Ruby 3.1's exception.message
(with additional information), I'll need to use exception.detailed_message(highlight: false)
(without ANSI sequences)?
Updated by mame (Yusuke Endoh) almost 3 years ago
Thank you for your comment!
st0012 (Stan Lo) wrote in #note-24:
I think it'll be fine but I'm not sure why the class name needs to be added as well?
The current error printer inserts the exception class name.
$ ruby -e 'raise "foo"'
-e:1:in `<main>': foo (RuntimeError)
This part (RuntimerError)
is emphasized by an underline, so the method responsible for highlight should insert it. Now Exception#detailed_message
has a responsibility to highlight the message, so it need to insert the class name.
$ ./miniruby -e 'begin; raise "foo"; rescue; p $!.detailed_message; end'
"foo (RuntimeError)"
$ ./miniruby -e 'begin; raise "foo"; rescue; p $!.detailed_message(highlight: true); end'
"\e[1mfoo (\e[1;4mRuntimeError\e[m\e[1m)\e[m"
But I agree that this is not a big deal.
And just for confirmation: so if I want to preserve the content of Ruby 3.1's
exception.message
(with additional information), I'll need to useexception.detailed_message(highlight: false)
(without ANSI sequences)?
Yes, you are right. More precisely, exception.respond_to?(:detailed_message) ? exception.detailed_message : exception.message
would work. Note that highlight
keyword is false by default.
If a feature to remove a class name is needed, maybe we need to write exception.detailed_message(exception_class_name: false)
or something.
Updated by Dan0042 (Daniel DeLorme) almost 3 years ago
mame (Yusuke Endoh) wrote in #note-25:
But I agree that this is not a big deal.
Exactly. At this point it's kind of a bikeshed, it's just that according to #note-1 Sentry displays the exception class above the message, so it would be displayed twice:
NoMethodError
undefined method `[]' for nil:NilClass (NoMethodError)
So if @st0012 (Stan Lo) wants to parse out the "(NoMethodError)", might as well not show it by default, and use detailed_message(exception_class_name: true)
in #full_message (the only place it's needed).
Updated by st0012 (Stan Lo) almost 3 years ago
Now Exception#detailed_message has a responsibility to highlight the message, so it need to insert the class name
I see. Thanks for the explanation!
maybe we need to write exception.detailed_message(exception_class_name: false) or something.
So if st0012 (Stan Lo) wants to parse out the "(NoMethodError)", might as well not show it by default, and use detailed_message(exception_class_name: true) in #full_message (the only place it's needed).
I don't think adding another option just for that is necessary. I'll first display the detailed_message
and if we receive any complain about the repeated class name, I'll remove it in the SDK.
Updated by mame (Yusuke Endoh) over 2 years ago
I talked with @matz (Yukihiro Matsumoto). He decided to go ahead with it as is, as it is not a serious problem.
If you want an exact compatibility with the traditional Exception#message
, please remove a class name from detailed_message.
exc = RuntimeError.new("foo\nbar\nbaz")
p exc.detailed_message #=> "foo (RuntimeError)\nbar\nbaz"
p exc.detailed_message.sub(/\A(.*?)(?: \(\w+\))/) { $1 } #=> "foo\nbar\nbaz"
We may consider adding a dedicated keyword argument for it if there are many requests.
Updated by mame (Yusuke Endoh) over 2 years ago
The following PR adds a guideline about usable escape sequences in Exception#detailed_message
.
https://github.com/ruby/ruby/pull/6119
Background:
An error message is primarily rendered in a terminal emulator, but is also shown in a browser by converting it to a HTML fragment, such as by Rack. However, the conversion would be unreasonably difficult if the message includes any escape sequence (such as cursor move or screen clear).
The safe and consertive way is to use highlight: false
, but @ioquatix (Samuel Williams) suggested that we also want a way to convert the text attributes to HTML. (https://github.com/rack/rack/pull/1925#issuecomment-1179470336)
I talked about the issue with @matz (Yukihiro Matsumoto), and we agreed to document a guideline for the use of escape sequences in Exception#detailed_message
:
- Use only widely-supported escape sequences: bold, underline, and basic eight foreground colors (except white and black).
- Make the message readable if all escape sequences are ignored.
I think these limitations would make conversion to HTML easier.
Updated by mame (Yusuke Endoh) about 2 years ago
- Status changed from Open to Closed
I have already merged the PR. Closing.