Hierarchical and Rate Limited Sampler Implementations #717

gillesbergerp · 2023-11-11T05:26:27Z

The hierarchical sampler allows for specifying a list of samplers that will be invoked in order. It will sample a request if any of its children would.
The Rate Limited sampler on the other hand samples requests at a fixed rate.

This is a prerequisite for #714

linux-foundation-easycla · 2023-11-11T05:26:33Z

The committers listed above are authorized under a signed CLA.

✅ login: gillesbergerp (2c92706, 43c5568, 1d033aa, c84a0da, 6e4ce7a, a6f2bf0, dcaccc2, d071542)

arielvalentin

This is a great start thank you!

Please add a stanza in our GH Actions to include tests for these gems:

https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/.github/workflows/ci-contrib.yml

https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/.github/workflows/ci-contrib-canary.yml

Also, please register the gems in the release list:

https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/.toys/.data/releases.yml

Lastly, please include the gems to the installation test suite here:

https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/releases/Gemfile

arielvalentin · 2023-11-13T18:00:15Z

sampling/hierarchical/README.md

+```
+
+Or, if you use [bundler][bundler-home], include `opentelemetry-sampling-hierarchical` in your `Gemfile`.
+


I think it would be helpful to add example usage in the README and/or an example directory with a script that shows how to configure it.

Unfortunately, there is no way yet to use it. This will require an update to https://github.com/open-telemetry/opentelemetry-ruby/blob/d365dfeb3982f09cd62a17c6e67eb3792f56e460/sdk/lib/opentelemetry/sdk/trace/tracer_provider.rb#L167. I will open a PR to support the X-Ray sampler there as well and will adjust the README afterwards accordingly if that's fine with you

sampling/hierarchical/lib/opentelemetry-sampling-hierarchical.rb

arielvalentin · 2023-11-13T18:04:47Z

sampling/hierarchical/lib/opentelemetry/sampling/hierarchical/sampler.rb

+
+module OpenTelemetry
+  module Sampling
+    module Hierarchical


Is Hierarchical Sampler a term used in X-Ray to describe this pattern? As opposed to a Any or All sampler?

It is not but I find Any sampler to be a bit misleading since it doesn't imply a fixed order.

I guess this name is not intuitive to me. Would you be able to help me understand what makes it Hierarchical or Ranked ordering vs "If any of the items in this list return true regardless of order"?

The list of samplers is iterated left to right. If any returns a positive response, we stop and say that the span should be sampled. Any doesn't give you this guarantee (in my interpretation).
The order can make a difference. A contrived example would be having two samplers:

Sampler A: one per second, only if kind == :internal

Sampler B: one per second, only if kind in [:server, :internal]

Let's say the application produces two traces in the same second, one with kind = :internal and one with kind = :server (in this order). If sampler A was applied first, both traces would be sampled. On the other hand, if sampler B was first, only the first (:internal) trace would be sampled since B already spent all its credits when the second trace arrives.

sampling/hierarchical/lib/opentelemetry/sampling/hierarchical/version.rb

arielvalentin · 2023-11-13T18:12:47Z

sampling/hierarchical/lib/opentelemetry/sampling/hierarchical/version.rb

+#
+# SPDX-License-Identifier: Apache-2.0
+
+module OpenTelemetry


I think the package the package naming should be symmetric with the SDK:

OpenTelemetry::SDK::Trace::Samplers

Or more use the API namespace

OpenTelemetry::Trace::Samplers

@open-telemetry/ruby-contrib-maintainers WDYT?

sampling/hierarchical/test/opentelemetry/sampling/hierarchical/sampler_test.rb

sampling/rate_limited/lib/opentelemetry/sampling/rate_limited/sampler.rb

sampling/rate_limited/lib/opentelemetry/sampling/rate_limited/version.rb

sampling/rate_limited/test/opentelemetry/sampling/rate_limited/sampler_test.rb

sampling/hierarchical/test/opentelemetry/test_helper.rb

robertlaurin · 2023-11-14T17:48:53Z

sampling/rate_limited/lib/opentelemetry/sampling/rate_limited/sampler.rb

+        # @param [Hash<String, Object>] attributes
+        # @return [OpenTelemetry::SDK::Trace::Samplers::Result] The sampling result.
+        def should_sample?(trace_id:, parent_context:, links:, name:, kind:, attributes:)
+          if can_spend?


Sampling is done per span so if you run out of credits_per_second mid trace you will start dropping spans, and potentially end up with broken traces.

Wouldn't this be better solved by instantiating a ParentBased sampler with the RateLimited sampler as root? I'D rather avoid making these distinctions here

What do you mean by making the parent based sampler as root?

Wouldn't that make it so the rate limited sampling result is always ignored since the Hierarchical Sampler will always check the other samplers to see if it should override the result?

I cannot follow. Are we talking about the hierarchical or rate-limited sampler?

robertlaurin · 2023-11-14T17:50:45Z

sampling/rate_limited/lib/opentelemetry/sampling/rate_limited/sampler.rb

+        def can_spend?
+          return false if @credits_per_second <= 0
+
+          @lock.synchronize do


The sampler is set on the tracer provider so you'll be locking every time you create a span, this may not be appropriate depending on how much span volume your service is generating.

Do you suggest being optimistic and not locking at all?

I don't think we have a suggestion as much as we're doing is raising concerns about the performance implications of adding another lock around span creation.

I'm not sure how much latency this introduces for applications that generate thousands of spans in a single transaction.

Since default thread counts are fairly low in most server implementations (in Puma it's 5, for instance for MRI). We can also provide a configuration option for that

robertlaurin · 2023-11-14T17:54:27Z

sampling/rate_limited/lib/opentelemetry/sampling/rate_limited/sampler.rb

+        private
+
+        # @return [Boolean]
+        def can_spend?


Do we care about how this performs in the case where an app is configured with Puma, with Unicorn? Forking vs Pre-forking web servers will result in different spend rates. What's the expected outcome here?

This is a good point that I haven't considered. I believe that the best option is to not make any assumptions about this at all. Configuring the rate accordingly could restore the parity between forking and pre-forking servers. Should we note this in the README?

It's more that I think we want clarity on what the expectations are for the scope of the rate limiter.

Does the rate limit apply per process, per thread, or per fiber?

Per process makes most sense, I believe. I don't see why different processes should have this knowledge of each other. It also makes the handling easier as we don't need to know the details of different servers

…ng-rate-limited in releases.yml

…using snake_case

github-actions · 2023-12-21T01:45:43Z

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

gillesbergerp requested review from fbogsany, mwear, robertlaurin, dazuma, ericmustin, arielvalentin, ahayworth, plantfansam, robbkidd, simi and kaylareopelle as code owners November 11, 2023 05:26

gillesbergerp requested a review from a team November 11, 2023 05:26

gillesbergerp mentioned this pull request Nov 11, 2023

Add option for X-Ray remote sampling #714

Closed

gillesbergerp force-pushed the feature/gillesbergerp/hierarchical-rate-limited branch from a40750d to be451b2 Compare November 11, 2023 05:30

arielvalentin reviewed Nov 13, 2023

View reviewed changes

robertlaurin reviewed Nov 14, 2023

View reviewed changes

Add the skeleton for a rate limited sampler

2c92706

gillesbergerp force-pushed the feature/gillesbergerp/hierarchical-rate-limited branch from be451b2 to 431f98e Compare November 17, 2023 10:06

gillesbergerp added 7 commits November 20, 2023 04:57

Implement a rate limited sampler

43c5568

Add the skeleton for a hierarchical sampler

1d033aa

Implement a hierarchical sampler

c84a0da

Add GitHub actions for sampling gems

6e4ce7a

Register opentelemetry-sampling-hierarchical and opentelemetry-sampli…

a6f2bf0

…ng-rate-limited in releases.yml

Add sampling gems to the installation test suite

dcaccc2

Ensure that the rate_limited gem adheres to the naming convention of …

d071542

…using snake_case

gillesbergerp force-pushed the feature/gillesbergerp/hierarchical-rate-limited branch from 431f98e to d071542 Compare November 20, 2023 03:58

github-actions bot added the stale Marks an issue/PR stale label Dec 21, 2023

github-actions bot closed this Jan 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hierarchical and Rate Limited Sampler Implementations #717

Hierarchical and Rate Limited Sampler Implementations #717

gillesbergerp commented Nov 11, 2023

linux-foundation-easycla bot commented Nov 11, 2023 •

edited

Loading

arielvalentin left a comment

arielvalentin Nov 13, 2023

gillesbergerp Nov 17, 2023

arielvalentin Nov 13, 2023

gillesbergerp Nov 17, 2023

arielvalentin Nov 19, 2023

gillesbergerp Nov 20, 2023 •

edited

Loading

arielvalentin Nov 13, 2023

robertlaurin Nov 14, 2023

gillesbergerp Nov 17, 2023

arielvalentin Nov 20, 2023

gillesbergerp Nov 20, 2023

robertlaurin Nov 14, 2023 •

edited

Loading

gillesbergerp Nov 17, 2023

arielvalentin Nov 20, 2023

gillesbergerp Nov 20, 2023

robertlaurin Nov 14, 2023

gillesbergerp Nov 17, 2023

arielvalentin Nov 20, 2023

gillesbergerp Nov 20, 2023

github-actions bot commented Dec 21, 2023

		```

		Or, if you use [bundler][bundler-home], include `opentelemetry-sampling-hierarchical` in your `Gemfile`.

Hierarchical and Rate Limited Sampler Implementations #717

Hierarchical and Rate Limited Sampler Implementations #717

Conversation

gillesbergerp commented Nov 11, 2023

linux-foundation-easycla bot commented Nov 11, 2023 • edited Loading

arielvalentin 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

gillesbergerp Nov 20, 2023 • edited Loading

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

robertlaurin Nov 14, 2023 • edited Loading

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

github-actions bot commented Dec 21, 2023

linux-foundation-easycla bot commented Nov 11, 2023 •

edited

Loading

gillesbergerp Nov 20, 2023 •

edited

Loading

robertlaurin Nov 14, 2023 •

edited

Loading