dd-trace-rb
dd-trace-rb copied to clipboard
Patch Sinatra only once
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!
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.
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.