Inconsistent instrumentation semantics

[jondot]

Hi,

I'd like to point out, and ask, what's the expected behavior when an exception is thrown in a Grape endpoint, towards instrumentation (via AS:N).

When I want to instrument Grape, I subscribe to this notification:

https://github.com/ruby-grape/grape/blob/master/lib/grape/endpoint.rb#L238

What I'm experiencing right now is that endpoint.status is always 200 (as opposed to 500 or default_error), even when an exception was raised in that endpoint.

When an exception is raised, I guess this blows up:

https://github.com/ruby-grape/grape/blob/master/lib/grape/endpoint.rb#L264

But then what happens with the instrumentation block?

When explicitly stating an error inside an endpoint, it works (status code is 500).
In a rescue_all block, or while an endpoint throws it doesn't work (status code is either stale, wasn't set, and is just 200)

[dblock]

It looks like a bug, would appreciate a spec that reproduces this to start.

[jondot]

@dblock this will fail: https://github.com/jondot/vitals/blob/b0b3e38ab02e0ce575916d910fca72a6868bf625/spec/vitals/integrations/notifications/grape_spec.rb#L64

while this will not:

https://github.com/jondot/vitals/blob/b0b3e38ab02e0ce575916d910fca72a6868bf625/spec/vitals/integrations/notifications/grape_spec.rb#L73

And this is the entirety of "instrumentation" code:

https://github.com/jondot/vitals/blob/master/lib/vitals/integrations/notifications/grape.rb#L12

[dblock]

Turn it into a spec, try to fix it.

[jondot]

I spent some time trying that, but I don't see any existing specs for AS::N that I can learn from in order to build a test in the existing Grape infra, however, tests in vitals would fail.
I'd prefer deprecating Grape support until we can find out if this is even a good use of the Grape AS::N instrumentation feature (which is what I'm asking).
Should we even grab the status from endpoint at that stage?

[dblock]

@jondot Are you talking about deprecating grape support in vitals? I can't speak to that.

In Grape the error handler sets status, it lives "outside" of the endpoint itself. So the answer I guess is "it depends on how an exception is being handled"?

[dim]

I would like to resurrect this issue.

@dblock I understand the reasoning, but shouldn't we then have and additional instrumentation middleware which sits outside of https://github.com/ruby-grape/grape/blob/3959a01f/lib/grape/endpoint.rb#L281. At the moment, there is absolutely no way to monitor the actual status returned by the server

[dblock]

I suppose the answer to having a way to monitor the actual status returned by the server is yes. So I would take a PR for that.