DEBUG-2334 duplicate mutable values when serializing for dynamic inst… #4009

p-datadog · 2024-10-17T16:05:59Z

…rumentation

Change log entry
None needed, DI is not usable by customers yet.

What does this PR do?

When serializing parameter values at method entry, the resulting snapshot should not change if the parameter is mutated during method execution. This currently only applies to string values as other values used as serializer output are not mutable.

This PR duplicates the string values which achieves immutability with no additional API complexity but at the cost of duplicating all strings, even if they are not entry parameters and thus can be stored without duplication.

Motivation:
Correct capture of method arguments at method entry.

Additional Notes:

How to test the change?
Unit tests at this time.

Unsure? Have a question? Request a review!

codecov-commenter · 2024-10-17T16:28:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.85%. Comparing base (f8d7b5e) to head (6b1ee84).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4009      +/-   ##
==========================================
- Coverage   97.86%   97.85%   -0.01%     
==========================================
  Files        1319     1319              
  Lines       79157    79195      +38     
  Branches     3929     3932       +3     
==========================================
+ Hits        77466    77499      +33     
- Misses       1691     1696       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pr-commenter · 2024-10-17T16:48:23Z

Benchmarks

Benchmark execution time: 2024-10-23 14:45:58

Comparing candidate commit 6b1ee84 in PR branch di-serializer-copy with baseline commit f8d7b5e in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 24 metrics, 2 unstable metrics.

…rumentation When serializing parameter values at method entry, the resulting snapshot should not change if the parameter is mutated during method execution. This currently only applies to string values as other values used as serializer output are not mutable. This commit duplicates the string values which achieves immutability with no additional API complexity but at the cost of duplicating all strings, even if they are not entry parameters and thus can be stored without duplication.

ivoanjo · 2024-10-18T03:11:49Z

lib/datadog/di/serializer.rb

        when String, Symbol
-          value = value.to_s
+          value = case value
+          when String
+            # This is the only place where we duplicate the value, currently.
+            # All other values are immutable primitives (e.g. numbers).
+            value.dup
+          else
+            value.to_s
+          end
          max = settings.dynamic_instrumentation.max_capture_string_length
          if value.length > max
            serialized.update(truncated: true, size: value.length)


May be worth using https://github.com/DataDog/dd-trace-rb/blob/master/lib/datadog/core/utils/safe_dup.rb or -v directly? 🤔

Also, it may be worth checking if truncation needs to happen first, otherwise we'll be creating yet another copy for the truncation 👀

Good point, if the string is to be truncated there is no need to dup it, and for frozen strings there is also no need to dup since they can't be mutated.

lib/datadog/di/serializer.rb

marcotc

~~I left a comment in case we can reuse the existing util.~~

Strech

👍🏼

Strech · 2024-10-22T09:36:01Z

lib/datadog/di/serializer.rb

+          value = case value
+          when String
+            # This is the only place where we duplicate the value, currently.
+            # All other values are immutable primitives (e.g. numbers).
+            # However, do not duplicate if the string is frozen, or if
+            # it is later truncated.
+            need_dup = !value.frozen?
+            value
+          else
+            value.to_s
+          end


We can turn case statement into if because it's just it

Suggested change

value = case value

when String

# This is the only place where we duplicate the value, currently.

# All other values are immutable primitives (e.g. numbers).

# However, do not duplicate if the string is frozen, or if

# it is later truncated.

need_dup = !value.frozen?

value

else

value.to_s

end

value = if value.is_a?(String)

# This is the only place where we duplicate the value, currently.

# All other values are immutable primitives (e.g. numbers).

# However, do not duplicate if the string is frozen, or if

# it is later truncated.

need_dup = !value.frozen?

value

else

value.to_s

end

Replaced case with if.

lib/datadog/di/serializer.rb

* origin/master: (51 commits) DEBUG-2334 duplicate mutable values when serializing for dynamic inst… (DataDog#4009) DEBUG-2334 enforce probe type validity (DataDog#4013) [🤖] Update Latest Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/11421728295 Fix the argument to the telemetry forwarder command [🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/11460992004 Add datadog gem to gemspec and remove from Gemfile Replace debase with datadog, and comment out gemspec tests Add datadog gem to Gemfile Remove debase gem from gemspec Use nix develop Use Ubuntu 24.04 by Arm Limited Fix vendored dependency case revert system-tests branch to main Changed RuleSampler initialization with ASM Standalone to Tracing::Component.build_sampler Rename AppSec::Event.add_tags to AppSec::Event.tag_and_keep! and move trace.keep in it Move appsec_standalone_reject? to AppSec namespace Replaced set_tag by set_metric for _dd.appsec.enabled and _dd.apm.enabled metrics Add correct sig to Datadog::AppSec::Event.add_tags and add_distributed_tags Update Unreleased Changelog Fix typo in AppSec::Event.add_tags spec ...

p-datadog requested a review from a team as a code owner October 17, 2024 16:06

p-datadog force-pushed the di-serializer-copy branch from a606869 to 604ed08 Compare October 17, 2024 16:07

p-datadog force-pushed the di-serializer-copy branch from 604ed08 to c52a21c Compare October 17, 2024 21:51

p-datadog force-pushed the di-serializer-copy branch from c52a21c to 95743ab Compare October 17, 2024 21:51

ivoanjo reviewed Oct 18, 2024

View reviewed changes

only duplicate strings if they are not frozen and not truncated

fc8bacd

marcotc reviewed Oct 18, 2024

View reviewed changes

lib/datadog/di/serializer.rb Show resolved Hide resolved

marcotc approved these changes Oct 18, 2024

View reviewed changes

Strech approved these changes Oct 22, 2024

View reviewed changes

p and others added 2 commits October 23, 2024 09:38

replace case with if

15f2d42

Merge branch 'master' into di-serializer-copy

6b1ee84

datadog-datadog-prod-us1 bot reviewed Oct 23, 2024

View reviewed changes

lib/datadog/di/serializer.rb Show resolved Hide resolved

p-datadog merged commit ee50bd2 into master Oct 23, 2024
269 of 270 checks passed

p-datadog deleted the di-serializer-copy branch October 23, 2024 15:06

github-actions bot added this to the 2.5.0 milestone Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEBUG-2334 duplicate mutable values when serializing for dynamic inst… #4009

DEBUG-2334 duplicate mutable values when serializing for dynamic inst… #4009

p-datadog commented Oct 17, 2024

codecov-commenter commented Oct 17, 2024 •

edited

Loading

pr-commenter bot commented Oct 17, 2024 •

edited

Loading

ivoanjo Oct 18, 2024

p-datadog Oct 18, 2024

p-datadog Oct 18, 2024

marcotc left a comment •

edited

Loading

Strech left a comment

Strech Oct 22, 2024

p-datadog Oct 23, 2024

DEBUG-2334 duplicate mutable values when serializing for dynamic inst… #4009

DEBUG-2334 duplicate mutable values when serializing for dynamic inst… #4009

Conversation

p-datadog commented Oct 17, 2024

codecov-commenter commented Oct 17, 2024 • edited Loading

Codecov Report

pr-commenter bot commented Oct 17, 2024 • edited Loading

Benchmarks

ivoanjo Oct 18, 2024

Choose a reason for hiding this comment

p-datadog Oct 18, 2024

Choose a reason for hiding this comment

p-datadog Oct 18, 2024

Choose a reason for hiding this comment

marcotc left a comment • edited Loading

Choose a reason for hiding this comment

Strech left a comment

Choose a reason for hiding this comment

Strech Oct 22, 2024

Choose a reason for hiding this comment

p-datadog Oct 23, 2024

Choose a reason for hiding this comment

codecov-commenter commented Oct 17, 2024 •

edited

Loading

pr-commenter bot commented Oct 17, 2024 •

edited

Loading

marcotc left a comment •

edited

Loading