DEBUG-3457 hack probe notifier worker starting #4358
Conversation
p-datadog
force-pushed
the
di-worker-middleware
branch
from
566ffc3 to
2bf7740
Compare
Datadog ReportBranch report: ✅ 0 Failed, 22035 Passed, 1477 Skipped, 5m 53.53s Total Time |
p-datadog
force-pushed
the
di-worker-middleware
branch
from
8ed2a67 to
63aea57
Compare
p-datadog
force-pushed
the
di-worker-middleware
branch
from
06540b7 to
95ad25b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4358 +/- ##
=======================================
Coverage 97.73% 97.74%
=======================================
Files 1347 1347
Lines 82388 82390 +2
Branches 4193 4195 +2
=======================================
+ Hits 80522 80528 +6
+ Misses 1866 1862 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The red configuration is either refusing to rerun or keeps failing, it's confusing. |
| @probe_notifier_worker = ProbeNotifierWorker.new(settings, transport, logger, telemetry: telemetry) | ||
| @probe_notification_builder = ProbeNotificationBuilder.new(settings, serializer) | ||
| @probe_manager = ProbeManager.new(settings, instrumenter, probe_notification_builder, probe_notifier_worker, logger, telemetry: telemetry) | ||
| probe_notifier_worker.start |
There was a problem hiding this comment.
Is there a reason the approach we use in tracing doesn't work for DI?
In tracing, we always check if the process has forked when submitting data, and if it has, we restart the worker:
dd-trace-rb/lib/datadog/tracing/writer.rb
Line 110 in 5319afc
There was a problem hiding this comment.
I can certainly do that.
| if defined?(Datadog::DI.current_component) | ||
| # TODO: when would Datadog::DI be defined but | ||
| # Datadog::DI.current_component not be defined? | ||
| Datadog::DI.current_component&.probe_notifier_worker&.start |
There was a problem hiding this comment.
Also a question similar to TODO's question: Is the current_component&.probe_notifier_worker needs to have safe-navigation? Could it be not defined?
There was a problem hiding this comment.
The entire DI component should be missing on Ruby 2.5 and JRuby. I am not sure (per the comment, and I did not investigate) why the DI module is defined but current_component is not defined sometimes. If current_component is defined and not nil, the rest of DI infrastructure should be loaded.
|
This PR was replaced by #4363. |
What does this PR do?
Moves the call to start probe notifier worker (i.e. the background thread) to tracing Rack middleware, which also starts remote config worker.
Motivation:
On forking web servers like puma the probe notifier worker is currently started in the parent (supervisor) process rather than in the worker processes, causing DI to not send any events to the agent.
Change log entry
Yes: fix dynamic instrumentation event sending on forking web servers
Additional Notes:
A proper solution would, in my opinion, not invoke DI (or remote config, which is a prerequisite for DI) from tracing, however in practice all DI customers should have tracing enabled therefore the solution in this PR should work for them.
How to test the change?
I manually verified it against debugger-demo-ruby.
Current CI does not appear to have a configuration that reproduces debugger-demo-ruby environment (since without this PR everything works in CI but does not work in debugger-demo-ruby).