-
Notifications
You must be signed in to change notification settings - Fork 376
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
DEBUG-3182 Rework DI loading #4239
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 22113 Passed, 1476 Skipped, 5m 57.55s Total Time |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4239 +/- ##
==========================================
- Coverage 97.73% 97.73% -0.01%
==========================================
Files 1352 1354 +2
Lines 82424 82449 +25
Branches 4232 4236 +4
==========================================
+ Hits 80558 80581 +23
- Misses 1866 1868 +2 ☔ View full report in Codecov by Sentry. |
If I can offer some advice on this, is as much as possible don't expose any API that's expected to be used directly by customers in DI. Specifically, since DI is mostly about "turn it on, then go to the datadog UX to clicky clicky", it's worth taking advantage of this by not allowing too much configuration/fiddling/and manual operations. This is somewhat similar to profiling: it's supported to be -- turn it on, then go look at the results. There's one or two public APIs but very very little surface, which has made it much easier to move fast and improve things without needing to walk on egg shells (or needing to put extra work or add extra overhead to comply with some specific public APIs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few notes!
* master: DEBUG-3210 DI: change logging to be appropriate for customer inspection (#4266) Report timing information if try_wait_until times out (#4265) Move simplecov patch to an overlay in our tree instead of using a forked simplecov repo (#4263) DEBUG-3251 dependency inject logger into DI component (#4262) DEBUG-3182 move Rails utils to core (#4261) add supported versions workflow (#4210) DEBUG-3305 remove dependency on benchmark (#4259) Fix case & grammar in issue template (#4244) [🤖] Update Latest Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/12614773826
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
We spoke via dm and while I'm somewhat concerned about the complexity around tracking the current component (locks, state to be tracked, potential memory leaks if not used correctly), this PR only moves something that already existed, so we can tackle that separately.
What does this PR do?
This PR separates the part of DI that is required for code tracker (which is the code tracker itself and the DI.current_component group of methods, plus exception classes) to be self-contained. Currently in master loading
datadog/di/init
would bring indatadog/di
as well which is too much (for example, that would cause DI to attempt to load ActiveRecord contrib, which is definitely not the right time for it).Motivation:
Ensuring DI code tracker is loaded early without causing the rest of dd-trace-rb to be loaded early.
Change log entry
Yes: Improved early loading mechanism of dynamic instrumentation (
datadog/di/init
).Additional Notes:
While this PR changes the behavior of
datadog/di/init
, this behavior has not yet been documented anywhere and should not be used by customers.How to test the change?
Integration tests specifically for
datadog/di/init
will be added in a follow-up PR. The existing unit tests verify that basic DI functionality continues to operate correctly.