search

open-telemetry / opentelemetry-ruby-contrib

Contrib Packages for the OpenTelemetry Ruby API and SDK implementation.
https://opentelemetry.io
Apache License 2.0
80 stars 164 forks source link

Sinatra instrumentation - Ensure http.route is set even when process exits early #1089

Closed lauraway closed 7 hours ago

lauraway commented 1 month ago

Currently the span name and http.route attribute are set in an ensure block and are not set until after the response is returned. There are cases where the process may exit early and leave the span name and attributes unset. One example of this is if a downstream service issues an exit command when a timeout occurs, thus ending the process. A change should be made to update the Sinatra instrumentation to set the span name before evaluating the route since the information is available prior to the request call.

https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/middlewares/tracer_middleware.rb#L21

xuan-cao-swi commented 1 month ago

If timeout occur, wouldn't it raise TimeoutError that can be caught by sinatra.error?

If it directly exit the (ruby) program when timeout occur, I suspect the span won't be exported at all.

arielvalentin commented 1 month ago

@xuan-cao-swi we have a bit of a special use case.

We have patched the otel-sdk to export spans in worker processes during a shutdown. The worker eventually kills the process using Kernel.exit!.

In these cases, we do indeed see the span because we finish it before exiting, however since the ensure block in the middleware does not complete the route is never set.

We would like to change the instrumentation to set as many attributes as possible before starting the request; that way in our scenario we will see the http.route and other request attributes set if the request is cancelled.

xuan-cao-swi commented 1 month ago

@arielvalentin do we just move L25-L29 before ensure?

arielvalentin commented 3 weeks ago

@lauraway do you think you will have time to work on this?

gfarb commented 1 day ago

After looking into this more, we have discovered that the proposed approach is not possible.

The middleware is processed before Sinatra::Application, which is where sinatra.route is getting set. Therefore, setting the span attributes before response = @app.call(env) will not work.

@lauraway do you have permissions to close this issue? If so, you can go ahead and close this out.