Skip to content
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

sample code of Sinatra integration does not work with classic style application #1935

Closed
okazu-dm opened this issue Mar 9, 2022 · 2 comments
Assignees
Labels
bug Involves a bug community Was opened by a community member integrations Involves tracing integrations

Comments

@okazu-dm
Copy link

okazu-dm commented Mar 9, 2022

I found sample code of Sinatra integration does not work if application is classic style.

I mean This sample code has a problem.
Sinatra classic style application start in at_exit
https://github.com/sinatra/sinatra/blob/2b5d1b45097f67c06a2345f0d8c30db2b38030bf/lib/sinatra/main.rb#L45
and Datadog::Writer.stop_worker method is called in at_exit
https://github.com/DataDog/dd-trace-rb/blob/v0.54.2/lib/ddtrace/writer.rb#L97

So current sample code means following execution order

  1. Datadog::Writer.stop_worker is executed
  2. Sinatra::Application.run!

(at_exit handlers are executed in reverser order of registration https://ruby-doc.org/core-3.1.1/Kernel.html#method-i-at_exit)

https://gist.github.com/okazu-dm/480f6188d2be7573b07c364cfac14194

@okazu-dm okazu-dm changed the title Sinatra integration does not work with classic style application sample code of Sinatra integration does not work with classic style application Mar 9, 2022
@delner
Copy link
Contributor

delner commented Mar 22, 2022

Thanks for the report!

I haven't dug into Sinatra's internals yet. Do you know why this at_exit hook exists in Sinatra code? I can see how the order issue causes problems, but it's not clear why 1) Sinatra is using this to start(?) an application and 2) why at_exit is triggered at all while starting up an application.

It's also surprising to me that this slipped through our unit tests. Something worth looking at why.

@delner delner self-assigned this Mar 22, 2022
@delner delner added bug Involves a bug integrations Involves tracing integrations community Was opened by a community member labels Mar 22, 2022
@TonyCTHsu
Copy link
Contributor

👋 @okazu-dm , have you resolved this issue? Recently, we have introduced a bunch of improvement at 1.6.0 for Sinatra, give it a try and tell us about it 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug community Was opened by a community member integrations Involves tracing integrations
Projects
None yet
Development

No branches or pull requests

3 participants