Wire Single Span Sampling by marcotc · Pull Request #2142 · DataDog/dd-trace-rb

marcotc · 2022-07-09T01:20:23Z

This PR finishes the Single Span Sampling feature, but creating a Tracing::Sampling::Span::Sampler instance and wiring it into the Tracer instance.

This PR also adds integration testing for the Single Span Sampling feature.

Other changes, done to finish up the feature:

Flushing and partial flushing are more complicated now: Single Sampled Spans has to always be flushed. This required changes to the Finished, Partial (and their CI counterparts) and TraceOperation classes. This part I'd like feedback on, as I'm not 100% confident on what the cleanest solution is here.
This PR cherry-picked the changes from Tell agent to trust tracer top-level span tagging #2138, as they are required for integration tests to pass. No need to review them here. This affects all changes to the following files: lib/ddtrace/transport/ext.rb, lib/ddtrace/transport/http.rb, and spec/ddtrace/transport/http_spec.rb.
Small change to lib/datadog/tracing/sampling/span/rule_parser.rb to improve resilience when receiving nil as it's main argument, something that can happen when sampling is not configured.

codecov-commenter · 2022-07-09T01:50:51Z

Codecov Report

Merging #2142 (ca91ac8) into single-span-fix-priority-sample (f926088) will decrease coverage by 0.00%.
The diff coverage is 99.62%.

@@                         Coverage Diff                         @@
##           single-span-fix-priority-sample    #2142      +/-   ##
===================================================================
- Coverage                            97.53%   97.53%   -0.01%     
===================================================================
  Files                                 1047     1047              
  Lines                                54410    54596     +186     
===================================================================
+ Hits                                 53070    53249     +179     
- Misses                                1340     1347       +7

Impacted Files	Coverage Δ
lib/datadog/tracing/trace_segment.rb	`100.00% <ø> (ø)`
spec/ddtrace/transport/http_spec.rb	`97.76% <ø> (ø)`
lib/datadog/tracing/metadata/tagging.rb	`95.00% <50.00%> (-2.37%)`	⬇️
lib/datadog/core/configuration/components.rb	`97.14% <100.00%> (+0.10%)`	⬆️
lib/datadog/tracing/flush.rb	`100.00% <100.00%> (ø)`
lib/datadog/tracing/sampling/span/rule_parser.rb	`100.00% <100.00%> (ø)`
lib/datadog/tracing/sampling/span/sampler.rb	`100.00% <100.00%> (ø)`
lib/datadog/tracing/span.rb	`97.70% <100.00%> (+0.05%)`	⬆️
lib/datadog/tracing/trace_operation.rb	`98.87% <100.00%> (+0.04%)`	⬆️
lib/ddtrace/transport/ext.rb	`100.00% <100.00%> (ø)`
... and 11 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

ivoanjo

Hey 👋

I've taken a stab at this one, from my usual POV of someone who knows less about the logic here and is trying to follow the intent of the change and the existing code.

Probably worth having someone else give an extra check because I definitely will have missed things :)

ivoanjo · 2022-07-12T08:52:30Z

        def full_flush?(trace_op)
-          trace_op && trace_op.sampled? && trace_op.finished?
+          trace_op && trace_op.finished?
        end


I assume this change (and the change below) needs to happen because now even if the trace has not been marked as sampled?, we may have some spans inside it that we want to flush.

But... this change is kinda weird. The whole point of this class (and partial, below) seems to be to determine "is this trace in a flushable state". But now that's basically a lie -- it's always going to say "yes", and then the actual decision of flushing is moved elsewhere.

I don't have a great suggestion here, other than I think these classes are so thin in logic that they should maybe folded back to the tracer class...(?)

I agree that for the Finished class, it's bare bones now. But the Partial one has legs still, and performs non-trivial work.

They don't look too great, as they know quite a lot about the TraceOperation internals. I'm not 100% if I have a good alternative right now.

ivoanjo · 2022-07-12T09:10:48Z

      Datadog::Tracing::TraceOperation,
      origin: origin,
-      sampled?: sampled,
      finished?: finished,
      flush!: trace
    )
  end

  let(:origin) { 'ci-origin' }
-  let(:sampled) { true }
  let(:finished) { true }



Is it just me or is Datadog::CI::Flush::Tagging a really bizarre part of the code? E.g. the class that is supposed to decide if something is ready for flushing or not has added behavior to mutate all spans to add an extra tag. Weeeird way of doing it... 👀

It seems like it's due to a lack of a clear interception point between the Tracing and CI products.
A CI formatter might make more sense.

ivoanjo · 2022-07-12T09:21:05Z

+        context 'with rule matching' do
+          context 'on name' do
+            context 'with a dropped span' do
+              let(:rules) { [{ name: 'my.op', sample_rate: 0.0 }] }
+
+              it_behaves_like 'flushed complete trace'
+              it_behaves_like 'does not modify spans'
+
+              context 'by rate limiting' do
+                let(:rules) { [{ name: 'my.op', sample_rate: 1.0, max_per_second: 0 }] }
+
+                it_behaves_like 'flushed complete trace'
+                it_behaves_like 'does not modify spans'
+              end
+            end
+
+            context 'with a kept span' do
+              let(:rules) { [{ name: 'my.op', sample_rate: 1.0 }] }
+
+              it_behaves_like 'flushed complete trace'
+              it_behaves_like 'set single span sampling tags'
+            end
+          end
+
+          context 'on service' do
+            context 'with a dropped span' do
+              let(:rules) { [{ service: 'my-ser*', sample_rate: 0.0 }] }
+
+              it_behaves_like 'flushed complete trace'
+              it_behaves_like 'does not modify spans'
+
+              context 'by rate limiting' do
+                let(:rules) { [{ service: 'my-ser*', sample_rate: 1.0, max_per_second: 0 }] }
+
+                it_behaves_like 'flushed complete trace'
+                it_behaves_like 'does not modify spans'
+              end
+            end
+
+            context 'with a kept span' do
+              let(:rules) { [{ service: 'my-ser*', sample_rate: 1.0 }] }
+
+              it_behaves_like 'flushed complete trace'
+              it_behaves_like 'set single span sampling tags'
+            end
+          end
+        end
+      end


This being an integration test, I'm not sure we need to exhaustively test all features of the rule matcher -- after all, we have tests for that, and we're just duplicating them.

Instead, I would suggest simplifying this to only testing a situation where the matches matches, and another where it doesn't. All other variants are effectively tests for the local logic in the sampler that don't otherwise reflect differently on tracing-as-a-whole.

Hmmm, I see duplication for sure.
I tried to match the somewhat higher level requirements from the RFC here in the integrations suite.

I'll see if I can simply these without removing too much in that I'm not as confident in the feature anymore.

Ok, I removed half the tests inside context 'with a dropped trace', specifically the distinct cases where rules could match based on span name or service name: now the tests only test if the span matches; the field that was used for matching is not important.

…ampling

marcotc · 2022-07-28T01:07:34Z

+                Datadog.logger.warn(
+                  "Error parsing Span Sampling Rules `#{rules.inspect}`: "\
+                                                      "#{e.class.name} #{e.message} at #{Array(e.backtrace).first}"
+                )


Rubocop 🤪

Yeah, sometimes I let it do its thing, and then come in after and tweak it so it's still "rubocop-acceptable" but does not look positively awful.

I'm still not convinced of the value of many of the rules in rubocop. Having a linter in general is ok, but rubocop is incredibly picky on irrelevant things, to the point of hindering productivity (at least for me).

Agreed. We should axe the rules that don't help. It's supposed to be an aid, not a hinderance.

I agree, I think there's a bug in Rubocop regarding multi line argument indentation.

TonyCTHsu

It generally make sense to me

TonyCTHsu · 2022-07-28T13:25:03Z

        # that has been accepted by trace-level sampling rules: all spans from such
        # trace are guaranteed to reach the Datadog App.
        class Sampler
+          attr_reader :rules


Is there a reason to make this public? I did not find other using this, or is this only a shortcut for test?

Short cut for testing.

The class itself is not part of the public API either.

delner

I have a number of questions I'd like to explore first before approving this. I wonder if we're coupling sampling logic too closely with spans (by putting sample/flush logic in the trace/span classes).

My concern is that I want to be able to re-use Span and TraceOperation as basic building blocks that are agnostic to the concept of sampling altogether in the future, if the future tracer decides not to sample.

delner · 2022-07-28T17:31:09Z

        end

        def full_flush?(trace_op)
-          trace_op && trace_op.sampled? && trace_op.finished?


Hmmm, why trace_op.sampled? omitted? Trying to figure out the purpose of this.

delner · 2022-07-28T17:34:43Z

+        # @param [String] tag the tag or metric to check for presence
+        # @return [Boolean] if the tag is present and not nil
+        def tag?(tag)
+          !get_tag(tag).nil? # nil is considered not present, thus we can't use `Hash#has_key?`


Is there a slightly more direct/performant way of doing this? Access the @ var itself? Or do we need to manage both the meta and metric vars?

On the fence about extending the API like this. I guess it's harmless, but it feels like adding in a test abstraction like this into production code seems not quite right. "Leaky." If there's any legit use of this outside the test suite, I think this is fine.

Otherwise, I think it would be wise to consider implementing such abstractions from within the test suite, if at all possible.

delner · 2022-07-28T17:35:03Z

            # @return [Array<Datadog::Tracing::Sampling::Span::Rule>] a list of parsed rules
            # @return [nil] if parsing failed
            def parse_json(rules)
+              return nil unless rules


Minor: nil is redundant?

delner · 2022-07-28T17:35:46Z

+                Datadog.logger.warn(
+                  "Error parsing Span Sampling Rules `#{rules.inspect}`: "\
+                                                      "#{e.class.name} #{e.message} at #{Array(e.backtrace).first}"
+                )


Agreed. We should axe the rules that don't help. It's supposed to be an aid, not a hinderance.

delner · 2022-07-28T17:38:14Z


+      # Single Span Sampling has chosen to keep this span
+      # regardless of the trace-level sampling decision
+      # @!visibility private


If this is meant to be private, can we move it behind the private keyword? I don't like soft rules like that which can be easily violated if you didn't read the docs.

My main concern is performance:

Warming up -------------------------------------- obj.foo 1.421M i/100ms obj.send(:foo) 363.874k i/100ms Calculating ------------------------------------- obj.foo 17.501M (± 2.3%) i/s - 53.987M in 3.086433s obj.send(:foo) 10.233M (± 3.0%) i/s - 30.929M in 3.025515s Comparison: obj.foo: 17500810.6 i/s obj.send(:foo): 10232543.6 i/s - 1.71x (± 0.00) slower

And how much of a sore obj.send(:foo) looks compared to obj.foo.

I'll change this method to be really private and use obj.send(:foo) when calling it in this PR.

delner · 2022-07-28T17:42:32Z

+      # Single Span Sampling has chosen to keep this span
+      # regardless of the trace-level sampling decision
+      # @!visibility private
+      def single_sampled?


I don't feel like this responsibility is quite right. Span should only be describing its own state, but now it seems to know something about the nature of span sampling. I don't like the presence of sampling logic and references in Span because I don't think it's a span's responsibility to know how it's sampled. It's supposed to be dumb.

It may be better to extract out this single_sampled? logic elsewhere, presumably to the thing that is aware of sampling that uses this. If this isn't possible/practical, I think at minimum this should be reduced to a simple variable e.g. @single_sampled.

delner · 2022-07-28T17:44:37Z

+        else
+          # Only spans where the span-level sampling overrides
+          # the trace-level sampling can be flushed.
+          flush_single_sampled_spans_only!


Similar to my feedback regarding Span and sampling, I think the same logic may apply here. Perhaps the better place for this kind of responsibility is in the Flush classes. They are supposed to know how to extract the "right" spans from an active trace.

delner · 2022-07-28T17:46:24Z

+      end
+
+      # Flush single sampled span only, as the trace as a whole was dropped by trace-level sampling
+      def flush_single_sampled_spans_only!


This logic seems very duplicative of the above/original logic. Should we be be adding an option to flush! that allows both of these paths to be driven based on optional input?

I addressed all points we discussed in person

marcotc · 2022-07-29T21:01:13Z

With all comments addressed, I'm going to merge this PR.

See you guys at our last stand: #2128!

Tell agent to trust trace top-level span tagging

5013c31

marcotc added the feature Involves a product feature label Jul 9, 2022

marcotc self-assigned this Jul 9, 2022

marcotc requested a review from a team July 9, 2022 01:20

Wire Single Span Sampling

ca91ac8

marcotc force-pushed the enable-single-span-sampling branch from bf7c6d7 to ca91ac8 Compare July 9, 2022 01:32

ivoanjo reviewed Jul 12, 2022

View reviewed changes

Base automatically changed from single-span-fix-priority-sample to feature-single-span-sampling July 28, 2022 00:05

marcotc added 4 commits July 27, 2022 17:21

Test Span#tag?

72eba19

Rename span name to make it easy to tell our test subject

558afb1

Simplify test

82628a6

Simply integration testing

601ee92

marcotc requested a review from ivoanjo July 28, 2022 01:01

marcotc added 2 commits July 27, 2022 18:03

Merge branch 'feature-single-span-sampling' into enable-single-span-s…

a454182

…ampling

Lint commit

3e9c7ca

marcotc commented Jul 28, 2022

View reviewed changes

marcotc requested a review from delner July 28, 2022 01:08

Hide documentation TODO until public docs are available

c066214

marcotc mentioned this pull request Jul 28, 2022

Single Span Sampling #2128

Merged

TonyCTHsu approved these changes Jul 28, 2022

View reviewed changes

delner previously requested changes Jul 28, 2022

View reviewed changes

Refactor flushing logic

339cd18

Sorbet

8f238ee

marcotc merged commit b171d31 into feature-single-span-sampling Jul 29, 2022

marcotc deleted the enable-single-span-sampling branch July 29, 2022 21:01

Uh oh!

Conversation

marcotc commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 9, 2022

Codecov Report

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcotc Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcotc Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivoanjo Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TonyCTHsu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcotc commented Jul 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

marcotc commented Jul 9, 2022 •

edited

Loading

marcotc Jul 28, 2022 •

edited

Loading

marcotc Jul 28, 2022 •

edited

Loading

ivoanjo Jul 28, 2022 •

edited

Loading