dd-trace-rb icon indicating copy to clipboard operation
dd-trace-rb copied to clipboard

Patch Sinatra only once

Open lloeki opened this issue 11 months ago • 2 comments

What does this PR do?

Sinatra needs to be patched once only.

Motivation:

Sinatra sets things up at first-request time, which may cause concurrent requests to call configure and break RC (thread leak).

Additional Notes:

The integration hooks onto Sinatra::Base#setup_middleware, which then (much later, as Sinatra::Base#setup_middleware appears to be invoked at first request) calls our Contrib::Sinatra::Framework.setup, which calls Datadog.configure and activate_rack! which invokes config.tracing.instrument :rack, which ultimately inserts the Rack middleware.

The trouble appears to be that this Datadog.configure call is not concurrency-protected enough, which leads to RC workers being started then mid-way the RC component being rebuilt with inconsistent access through active_remote.

How to test the change?

  • CI
  • test in a Sinatra app

For Datadog employees:

  • [ ] If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • [x] This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

lloeki avatar Mar 08 '24 13:03 lloeki

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.23%. Comparing base (44b9c6c) to head (9164ae1). Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3515      +/-   ##
==========================================
- Coverage   98.23%   98.23%   -0.01%     
==========================================
  Files        1275     1275              
  Lines       75226    75228       +2     
  Branches     3552     3552              
==========================================
+ Hits        73901    73902       +1     
- Misses       1325     1326       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Mar 08 '24 14:03 codecov-commenter

There are two things happening here that should be split:

  • setting the configuration settings for Sinatra+Rack, which should be done immediately (not delayed to a later time):
    • saying that this integration is enabled
    • setting any passed options, or setting to default values
  • inserting our middlewares into the Rack stack, which can only be done when there is a Rack stack (which Sinatra appears to do lazily, hence the hook on setup_middlewares and the cause of this happening when requests start to be received)

The former goes through the configure block, the latter should be done outside of a configure block.

IOW THERE SHOULD BE NO CALL TO CONFIGURE AT APP RUNTIME (SERVING REQUESTS). The design of configure is just not amenable to that currently.

That would go a long way towards this class of issue disappearing.

lloeki avatar Mar 08 '24 14:03 lloeki