Add meta_struct support on traces and spans #4334

vpellan · 2025-01-31T12:27:40Z

What does this PR do?

This PR adds meta_struct support to traces and spans. meta_struct is a way to send complex structures to the backend. It is sent as a Map<String, ByteArray> with the ByteArray being the structure encoded in messagepack.

Motivation:

meta_struct is required for stack trace collection, which is required for exploit prevention. But this can also be used by other products.

Change log entry

None.

Additional Notes:

How to test the change?

datadog-datadog-prod-us1 · 2025-01-31T12:49:44Z

Datadog Report

Branch report: vpellan/meta-struct
Commit report: 65dd37b
Test service: dd-trace-rb

✅ 0 Failed, 22095 Passed, 1476 Skipped, 5m 3.14s Total Time

pr-commenter · 2025-01-31T13:03:33Z

Benchmarks

Benchmark execution time: 2025-02-06 12:54:53

Comparing candidate commit 65dd37b in PR branch vpellan/meta-struct with baseline commit a70cf66 in branch master.

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

scenario:tracing - 1 span trace - no writer

🟥 throughput [-724.190op/s; -682.558op/s] or [-5.511%; -5.195%]

scenario:tracing - 10 span trace - no network

🟥 throughput [-348.050op/s; -145.573op/s] or [-13.885%; -5.807%]

scenario:tracing - 10 span trace - no writer

🟥 throughput [-185.743op/s; -179.304op/s] or [-6.477%; -6.252%]

scenario:tracing - 100 span trace - no writer

🟥 throughput [-25.773op/s; -25.133op/s] or [-7.428%; -7.244%]

codecov-commenter · 2025-01-31T13:06:24Z

Codecov Report

Attention: Patch coverage is 92.92035% with 8 lines in your changes missing coverage. Please review.

Project coverage is 97.71%. Comparing base (a70cf66) to head (65dd37b).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/datadog/tracing/metadata/metastruct.rb	86.66%	4 Missing ⚠️
lib/datadog/tracing/span_operation.rb	33.33%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4334      +/-   ##
==========================================
- Coverage   97.72%   97.71%   -0.02%     
==========================================
  Files        1368     1370       +2     
  Lines       82998    83106     +108     
  Branches     4220     4227       +7     
==========================================
+ Hits        81113    81209      +96     
- Misses       1885     1897      +12

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

Strech

Awesome work 👏🏼

I think we can run a round of light improvements over the naming and interfaces, rest looks solid!

Strech · 2025-02-03T14:41:29Z

lib/datadog/tracing/metadata/meta_struct.rb

+      # @public_api
+      module MetaStruct
+        def set_meta_struct(meta_struct)
+          self.meta_struct.merge!(meta_struct)


I think the name prefix set_ implies that any data prior to that call will be overwritten.

Fixed in 0066df9 (following up @marcotc comment)

Strech · 2025-02-03T14:44:08Z

lib/datadog/tracing/transport/serializable_trace.rb

@@ -113,6 +113,9 @@ def to_msgpack(packer = nil)
          packer.write(span.meta)
          packer.write('metrics')
          packer.write(span.metrics)
+          packer.write('meta_struct')
+          # We encapsulate the resulting msgpack in a binary msgpack
+          packer.write(span.meta_struct.transform_values(&:to_msgpack))


If you want to encapsulate, then the logic of the values transformation should not be exposed, instead to_msgpack should be called on entire meta_struct

packer.write(span.meta_struct.to_msgpack)

btw, to_msgpack is implicitly called by packer.write, if present.

But packer.write(span.meta_struct.transform_values(&:to_msgpack)) is ok.

Yes, to_msgpack is implicitly called by packer.write but we need to call it a second time, so that the values are encoded as byte array. Or else they would be maps or arrays, but the backend wants byte arrays.

Maybe my comment is not descriptive enough. We encapsulate the values, not the whole hash, in a message pack (which is why we use transform values) which will give a String starting with something like \xDE or \xDF. Then this String will be converted to byte array because of packer.write. span.metastruct is just a Hash, so using span.metastruct.to_msgpack would give the wrong result as we only want to convert the values to message pack, not the keys. Do you think it would be preferable to use an external method, like convert_values_to_msgpack(span.metastruct) ?

Strech · 2025-02-03T14:45:00Z

sig/datadog/tracing/span.rbs

@@ -6,13 +6,14 @@ module Datadog
      attr_accessor end_time: (Time | nil)
      attr_accessor id: Integer
      attr_accessor meta: Hash[String, String]
+      attr_accessor meta_struct: Hash[String, untyped]


That should be a type alias inside the MetaStruct rbs instead

Fixed in f36e147

Strech · 2025-02-03T14:45:37Z

sig/datadog/tracing/trace_operation.rbs

@@ -23,7 +23,7 @@ module Datadog
      attr_writer sampled: untyped
      attr_writer service: untyped

-      def initialize: (?agent_sample_rate: untyped?, ?events: untyped?, ?hostname: untyped?, ?id: untyped?, ?max_length: untyped, ?name: untyped?, ?origin: untyped?, ?parent_span_id: untyped?, ?rate_limiter_rate: untyped?, ?resource: untyped?, ?rule_sample_rate: untyped?, ?sample_rate: untyped?, ?sampled: untyped?, ?sampling_priority: untyped?, ?service: untyped?, ?tags: untyped?, ?metrics: untyped?, ?remote_parent: untyped?) -> void
+      def initialize: (?agent_sample_rate: untyped?, ?events: untyped?, ?hostname: untyped?, ?id: untyped?, ?max_length: untyped, ?name: untyped?, ?origin: untyped?, ?parent_span_id: untyped?, ?rate_limiter_rate: untyped?, ?resource: untyped?, ?rule_sample_rate: untyped?, ?sample_rate: untyped?, ?sampled: untyped?, ?sampling_priority: untyped?, ?service: untyped?, ?tags: untyped?, ?meta_struct: untyped?, ?metrics: untyped?, ?remote_parent: untyped?) -> void


I think for the new class you can use a type alias from MetaStruct

Fixed in f36e147

Strech · 2025-02-03T14:47:44Z

lib/datadog/tracing/metadata/meta_struct.rb

+          self.meta_struct.merge!(meta_struct)
+        end
+
+        def get_meta_struct(key)


Now sure about naming, but if I call get_meta_struct I would expect the whole meta-struct object in return. I think we should somehow highlight that it's a part inside the meta-struct.

Maybe names like

get_meta_struct_value get_meta_struct_field

or something way better (see ⬇️)

Strech · 2025-02-03T14:49:20Z

lib/datadog/tracing/metadata/meta_struct.rb

+    module Metadata
+      # Adds complex structures tagging behavior through meta_struct
+      # @public_api
+      module MetaStruct


I have a question about naming, we call Metadata as a single word, but MetaStruct as two words. Does it make sense if we call it Metastruct? We can avoid long names on reading too

get_metastruct_value("key")

Fixed in 7d76a8e

p-datadog

Requesting changes due to mismatch between no change log entry claimed in PR description and the new API being marked public.

lib/datadog/tracing/metadata/meta_struct.rb

marcotc · 2025-02-03T22:41:15Z

lib/datadog/tracing/metadata/meta_struct.rb

+      # Adds complex structures tagging behavior through meta_struct
+      # @public_api
+      module MetaStruct
+        def set_meta_struct(meta_struct)


@Strech why not def meta_struct= here and def meta_struct(key) for reading?

hmm, because it's merge eh... this is in interesting api

Ok, how is this API going to be used? By key? In batch? Is the "read" ever invoked?

This method is used in two places:

during TraceOperation initialisation (itself used in fork_clone) which just sets metastruct to what is given in initialize. metastruct is empty in this case and the behaviour is corresponding to metastruct=.

in TraceFormatter, when we merge the metastruct contained in the TraceOperation with the one contained in the root span. And in our case, we don't want to replace an existing field, but merge it too, so effectively we want a deep_merge (which is not the behaviour we have right now).

So I'm going to split that method in two, metastruct= and deep_merge_metastruct!

What do you think about that ? 0066df9

datadog-datadog-prod-us1 · 2025-02-04T16:44:20Z

lib/datadog/tracing/metadata/metastruct.rb

+          merger = proc do |_, v1, v2|
+            if v1.is_a?(Hash) && v2.is_a?(Hash)
+              v1.merge(v2, &merger)
+            elsif v1.is_a?(Array) && v2.is_a?(Array)


⚪ Code Quality Violation

Consider using Array() to ensure the type is that of an array (...read more)

The rule "Use Array() to ensure your variable is an array" is important for ensuring your code behaves as expected, regardless of the type of data it receives. It is common in Ruby to need to iterate through an array of items. However, if the variable is not an array, this can lead to unexpected behavior or errors.

The Array() method in Ruby is a Kernel method that converts its argument to an Array. If the argument is already an Array, it returns the argument. If the argument is nil, it returns an empty Array. This can be used to ensure that a variable is an array before trying to iterate over it, preventing potential errors or unexpected behavior.

By using Array(foos), you can ensure that foos is an array before you try to iterate over it with each. This prevents the need to check if foos is an array with foos.is_a?(Array) and makes your code cleaner and easier to understand.

…and TraceOperation

Split set_metastruct into deep_merge_metastruct! and metastruct=

repaired

Strech

It depends on the tracing team, but I've left a few notes following our discussion about the metastruct implementation

Strech · 2025-02-05T17:01:55Z

lib/datadog/tracing/metadata/metastruct.rb

+          metastruct[key]
+        end
+
+        protected


I would go with private if we don't expose it outside, protected doesn't make much sense in Ruby.

Strech · 2025-02-05T17:06:07Z

lib/datadog/tracing/metadata/metastruct.rb

+        def metastruct=(second)
+          @metastruct = second
+        end


Is this method used? I can't find, should we remove it?

It is used in TraceOperation initialisation, which itself is used by fork_clone

Strech · 2025-02-05T17:11:22Z

lib/datadog/tracing/metadata/metastruct.rb

+        def get_metastruct_field(key)
+          metastruct[key]
+        end


I know at the first iteration we can keep it simple, but honestly it feels like the prefixing is needed only because we don't give it as an object, just to showcase it:

mst = meta.metastruct # => Metadata::MetastructObject mst['my-key'] # => {...} mst['my-key'] = {...} # => {...} mst.deep_merge!({...}) # => Metadata::MetastructObject

And then we can do for MetastructObject all sorts of encapsulation to hide the logic

class MetastructObject def []=(key) # ... end end

We have discussed it, but just to give you perspective of how the interactions and naming could be eased with an object instead of prefixes

What do you think about that ? 65dd37b

lib/datadog/tracing/metadata/metastruct.rb

Strech

Amazing move, I think it's good to go! Solid work 👏🏼

P.S I left few suggestions, but they are not blockers

Strech · 2025-02-06T16:53:32Z

lib/datadog/tracing/metadata/metastruct.rb

+        def initialize(metastruct = nil)
+          @metastruct = metastruct
+        end


A small suggestion, you can get rid of metastruct method if you guarantee in the initializer the success of the storage (metastruct)

Suggested change

def initialize(metastruct = nil)

@metastruct = metastruct

end

def initialize(metastruct = nil)

@metastruct = metastruct || {}

end

Strech · 2025-02-06T16:54:54Z

lib/datadog/tracing/metadata/metastruct.rb

+              v2
+            end
+          end
+          metastruct.merge!(second.to_h, &merger) # steep:ignore BlockTypeMismatch


Are you sure that we need to convert second into Hash before merging?

Strech · 2025-02-06T17:00:46Z

spec/datadog/tracing/trace_operation_spec.rb

+      it do
+        expect(trace_op.send(:metastruct).to_h).to eq({})
+      end


Suggested change

it do

expect(trace_op.send(:metastruct).to_h).to eq({})

end

it { expect(trace_op.send(:metastruct).to_h).to eq({}) }

github-actions bot added appsec Application Security monitoring product tracing labels Jan 31, 2025

vpellan force-pushed the vpellan/meta-struct branch 2 times, most recently from b3dc5c2 to 0836e11 Compare January 31, 2025 12:48

vpellan removed the appsec Application Security monitoring product label Jan 31, 2025

vpellan force-pushed the vpellan/meta-struct branch from faf7819 to fcda7ca Compare February 3, 2025 14:27

vpellan marked this pull request as ready for review February 3, 2025 14:30

vpellan requested a review from a team as a code owner February 3, 2025 14:30

Strech reviewed Feb 3, 2025

View reviewed changes

p-datadog previously requested changes Feb 3, 2025

View reviewed changes

lib/datadog/tracing/metadata/meta_struct.rb Outdated Show resolved Hide resolved

marcotc reviewed Feb 3, 2025

View reviewed changes

datadog-datadog-prod-us1 bot reviewed Feb 4, 2025

View reviewed changes

vpellan added 3 commits February 4, 2025 18:21

Add meta_struct support on traces and spans

9a23f15

Add get_meta_struct and meta_struct_size methods to MetaStruct class …

d4bbb51

…and TraceOperation

Add meta_struct specs

95bc08e

vpellan force-pushed the vpellan/meta-struct branch from 0c00cb0 to 7d76a8e Compare February 4, 2025 17:29

marcotc approved these changes Feb 4, 2025

View reviewed changes

vpellan force-pushed the vpellan/meta-struct branch from 7d76a8e to 9b62558 Compare February 5, 2025 09:03

vpellan added 4 commits February 5, 2025 10:15

Rename meta_struct into metastruct

c172eac

Remove metastruct_size (no usage)

3ffcd14

Split set_metastruct into deep_merge_metastruct! and metastruct=

Add metastruct type

aa4b999

Rename get_metastruct to get_metastruct_field

52d5a77

vpellan force-pushed the vpellan/meta-struct branch from 9b62558 to 52d5a77 Compare February 5, 2025 09:22

Add set_metastruct_field method

26f1f6d

Strech reviewed Feb 5, 2025

View reviewed changes

y9v reviewed Feb 6, 2025

View reviewed changes

lib/datadog/tracing/metadata/metastruct.rb Outdated Show resolved Hide resolved

vpellan force-pushed the vpellan/meta-struct branch from b29e3fb to a771a6d Compare February 6, 2025 12:30

Change Metastruct from module to class

65dd37b

vpellan force-pushed the vpellan/meta-struct branch from a771a6d to 65dd37b Compare February 6, 2025 12:31

Strech approved these changes Feb 6, 2025

View reviewed changes

y9v approved these changes Feb 7, 2025

View reviewed changes

Add meta_struct support on traces and spans #4334

Are you sure you want to change the base?

Add meta_struct support on traces and spans #4334

Conversation

vpellan commented Jan 31, 2025

datadog-datadog-prod-us1 bot commented Jan 31, 2025 • edited Loading

Datadog Report

pr-commenter bot commented Jan 31, 2025 • edited Loading

Benchmarks

scenario:tracing - 1 span trace - no writer

scenario:tracing - 10 span trace - no network

scenario:tracing - 10 span trace - no writer

scenario:tracing - 100 span trace - no writer

codecov-commenter commented Jan 31, 2025 • edited Loading

Codecov Report

Strech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strech Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Strech Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-datadog left a comment

Choose a reason for hiding this comment

marcotc Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot Feb 4, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Strech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Jan 31, 2025 •

edited

Loading

pr-commenter bot commented Jan 31, 2025 •

edited

Loading

codecov-commenter commented Jan 31, 2025 •

edited

Loading

Strech Feb 3, 2025 •

edited

Loading

Strech Feb 3, 2025 •

edited

Loading

marcotc Feb 3, 2025 •

edited

Loading

marcotc Feb 3, 2025 •

edited

Loading