[tracer:stacktrace] fix error stack traces #170
Conversation
lib/ddtrace.rb
Outdated
|
|
||
| module Datadog | ||
| # Railtie class initializes | ||
| class Railtie < Rails::Railtie | ||
| config.app_middleware.use(Datadog::Contrib::Rails::TraceMiddleware) |
There was a problem hiding this comment.
It doesn't conflict with https://github.com/DataDog/dd-trace-rb/pull/170/files#diff-f9f184a4f121cd58a4162819f69200f7R57?
There was a problem hiding this comment.
Ouch saw that it's Rails and not Rack. Can we call it ExceptionMiddleware or similar if it's handling only exceptions?
| @@ -128,10 +129,6 @@ def call(env) | |||
| # unless it has been already set by the underlying framework | |||
| if status.to_s.start_with?('5') && request_span.status.zero? | |||
| request_span.status = 1 | |||
| # in any case we don't touch the stacktrace if it has been set | |||
There was a problem hiding this comment.
so this one sets wrong stack traces and removing it means writing this one for each framework built on top of Rack, right?
There was a problem hiding this comment.
Well, the code below has been removed because it was only a fallback "in case we don't have the exception" trying to fill the stack trace with something. It appears it was a bad idea, it confuses users, as the stack trace is only about... our code, which provides no insight for users to spot where the problem actually was. If the error was catched by another layer, we just do not have the information any more, period, so rather than report misleading info, report nothing.
There was a problem hiding this comment.
Totally agree. Let's remove it!
| # and it is user code which should be executed no matter what. | ||
| # It's not a problem since we re-raise it afterwards so for example a | ||
| # SignalException::Interrupt would still bubble up. | ||
| rescue Exception => e |
There was a problem hiding this comment.
Till we re-raise e, I'm ok with using Exception. Also yes, this is the right thing to do at this point.
Rails instrumentation does not report correct stack traces (see #110 which also referred to this point). This patch introduces a new way to populate this stack trace (heavily inspired by @Ferdy89 work), not relying on
callerany more which, indeed, is not want we want.