DEBUG-2334 remaining dynamic instrumentation code #4098
Conversation
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
spec/datadog/di/contrib/active_record/serializer_active_record_spec.rb
Outdated
Show resolved
Hide resolved
BenchmarksBenchmark execution time: 2024-11-25 00:05:16 Comparing candidate commit 4683a48 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 29 metrics, 2 unstable metrics. scenario:profiler - sample timeline=false
scenario:tracing - Propagation - Trace Context
|
p-datadog
force-pushed
the
di-rest-2
branch
2 times, most recently
from
b7e93a0
to
288d1d8
Compare
p-datadog
force-pushed
the
di-rest-2
branch
5 times, most recently
from
79e1ff8
to
7d4aedf
Compare
p-datadog
force-pushed
the
di-rest-2
branch
5 times, most recently
from
66546cd
to
52034e2
Compare
p-datadog
force-pushed
the
di-rest-2
branch
3 times, most recently
from
3f462a9
to
7f94a88
Compare
Maximum capture attribute count can be specified in the probe, implement support for it in DI.
lib/datadog/di.rb
Outdated
| if RUBY_VERSION >= '2.6' | ||
| begin | ||
| # Activate code tracking by default because line trace points will not work | ||
| # without it. | ||
| Datadog::DI.activate_tracking! | ||
| rescue => exc | ||
| if defined?(Datadog.logger) | ||
| Datadog.logger.warn("Failed to activate code tracking for DI: #{exc.class}: #{exc}") | ||
| else | ||
| # We do not have Datadog logger potentially because DI code tracker is | ||
| # being loaded early in application boot process and the rest of datadog | ||
| # wasn't loaded yet. Output to standard error. | ||
| warn("Failed to activate code tracking for DI: #{exc.class}: #{exc}") | ||
| end | ||
| end |
There was a problem hiding this comment.
Small nit:
| if RUBY_VERSION >= '2.6' | |
| begin | |
| # Activate code tracking by default because line trace points will not work | |
| # without it. | |
| Datadog::DI.activate_tracking! | |
| rescue => exc | |
| if defined?(Datadog.logger) | |
| Datadog.logger.warn("Failed to activate code tracking for DI: #{exc.class}: #{exc}") | |
| else | |
| # We do not have Datadog logger potentially because DI code tracker is | |
| # being loaded early in application boot process and the rest of datadog | |
| # wasn't loaded yet. Output to standard error. | |
| warn("Failed to activate code tracking for DI: #{exc.class}: #{exc}") | |
| end | |
| end | |
| return if RUBY_VERSION >= '2.6' | |
| # Activate code tracking by default because line trace points will not work | |
| # without it. | |
| Datadog::DI.activate_tracking! | |
| rescue => exc | |
| if defined?(Datadog.logger) | |
| Datadog.logger.warn("Failed to activate code tracking for DI: #{exc.class}: #{exc}") | |
| else | |
| # We do not have Datadog logger potentially because DI code tracker is | |
| # being loaded early in application boot process and the rest of datadog | |
| # wasn't loaded yet. Output to standard error. | |
| warn("Failed to activate code tracking for DI: #{exc.class}: #{exc}") | |
| end |
There was a problem hiding this comment.
I think you meant to reverse the sign on comparison which I did.
lib/datadog/di.rb
Outdated
| end | ||
| end | ||
| =end | ||
| Datadog::DI.activate_tracking |
There was a problem hiding this comment.
It is unusual to see method calls done at the root level, when the file is loaded.
Can you add a short comment here explaining why it's needed at this level? I understand that it's an early activation needed for DI, and that there's a more expansive explanation of DI behavior somewhere else in code, but just leave a short note here so people don't get confused about this call site.
There was a problem hiding this comment.
Added a comment and this is now conditional on DI environment variable being set.
lib/datadog/di/remote.rb
Outdated
| # TODO when would this happen? Do we request DI RC when DI is not on? | ||
| # Or we always get RC even if DI is not turned on? | ||
| # -- in dev. env when component is not built but config is still requested? | ||
| # TODO log something? |
There was a problem hiding this comment.
I don't think you should have this remote configuration class receiving data if DI's component was never initialize.
I'd say we can assume that this is an impossible case. It makes sense to check if DI's component is always initialized early (which I think it is).
There was a problem hiding this comment.
Adjusted the comment and maybe we can inject the DI component into here at some point in the future instead of having to reference a global. I think this is the only place besides code tracker (which necessarily must reference a global, since normally it's created before DI component exists at all) that doesn't have all of its dependencies injected.
| # we will start the new remote component as well. | ||
| old_state = { | ||
| remote_started: old.remote&.started?, | ||
| } |
There was a problem hiding this comment.
@marcotc I added a note here per your comment in the other PR, does it sound OK to you?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4098 +/- ##
==========================================
+ Coverage 97.76% 97.77% +0.01%
==========================================
Files 1350 1353 +3
Lines 81327 81714 +387
Branches 4107 4139 +32
==========================================
+ Hits 79508 79898 +390
+ Misses 1819 1816 -3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
What does this PR do?
is set in environmentMotivation:
Initial DI implementation in Ruby
Change log entry
Dynamic instrumentation is now available in Ruby. Currently only log probes are implemented, but they can be set on both methods and lines.
Additional Notes:
Replaces #4063
How to test the change?
Additional unit and integration tests are included + system tests in DataDog/system-tests#3516