Composite Samplers prototype #1443

PeterF778 · 2024-08-30T00:29:26Z

This PR modifies the Consistent Probability Samplers to make them conform to the proposed Composite Samplers at open-telemetry/oteps#250

More work is planned: The classes ConsistentSampler and ComposableSampler should be probably merged into one.

This PR modifies the Consistent Probability Samplers to make them conform to the proposed Composite Samplers at open-telemetry/oteps#250

...-sampling/src/main/java/io/opentelemetry/contrib/sampler/consistent56/ComposableSampler.java

...ent-sampling/src/main/java/io/opentelemetry/contrib/sampler/consistent56/SamplingIntent.java

...istent-sampling/src/test/java/io/opentelemetry/contrib/sampler/consistent56/UseCaseTest.java

.../src/main/java/io/opentelemetry/contrib/sampler/consistent56/ConsistentRuleBasedSampler.java

...c/main/java/io/opentelemetry/contrib/sampler/consistent56/ConsistentRateLimitingSampler.java

...nt-sampling/src/main/java/io/opentelemetry/contrib/sampler/consistent56/ConsistentAnyOf.java

…pler/consistent56/ComposableSampler.java Co-authored-by: Otmar Ertl <[email protected]>

…pler/consistent56/SamplingIntent.java Co-authored-by: Otmar Ertl <[email protected]>

…pler/consistent56/UseCaseTest.java Co-authored-by: Otmar Ertl <[email protected]>

…pler/consistent56/ConsistentRuleBasedSampler.java Co-authored-by: Otmar Ertl <[email protected]>

…pler/consistent56/ConsistentAnyOf.java Co-authored-by: Otmar Ertl <[email protected]>

…pler/consistent56/ConsistentRuleBasedSampler.java Co-authored-by: Otmar Ertl <[email protected]>

…pler/consistent56/CoinFlipSampler.java Co-authored-by: Otmar Ertl <[email protected]>

…pler/consistent56/ConsistentAnyOf.java Co-authored-by: Otmar Ertl <[email protected]>

jmacd · 2024-09-06T21:28:33Z

...main/java/io/opentelemetry/contrib/sampler/consistent56/ConsistentFixedThresholdSampler.java

+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.sdk.trace.data.LinkData;
+import java.util.List;

 public class ConsistentFixedThresholdSampler extends ConsistentSampler {


This looks and feels like what the OTel Trace SDK TraceIdRatioBased sampler wants to be. 👍

jmacd · 2024-09-06T21:36:26Z

...main/java/io/opentelemetry/contrib/sampler/consistent56/ConsistentFixedThresholdSampler.java

+      if (threshold == getMaxThreshold()) {
+        return getInvalidThreshold();
+      }


Please comment, I had to go review https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/consistent-sampling/src/main/java/io/opentelemetry/contrib/sampler/consistent56/ConsistentSamplingUtil.java to understand that getMaxThreshold() is what I was expecting getInvalidThreshold() to be.

Why does the Util package distinguish Max and Invalid? (Why aren't they a single value, maybe?)

This probably requires a cleanup. The prototype currently uses long rather than hex String to represent the thresholds, but the proposed specs allow both implementations. I'd love to see the same logic and behavior for all the new samplers regardless of the choice of the threshold representation. However, the issue is with the MaxThreshold, which is really an out-of-range value. Representing the maximum threshold as a hex string is not useful, as it would be a 15-character string and it would break lexicographical comparisons.
Therefore I opted into requiring that getSamplingIntent always provides a valid threshold (which is smaller than 2^56), or an invalid one, which is a special value easy to test for.

jmacd · 2024-09-06T21:37:46Z

...rc/main/java/io/opentelemetry/contrib/sampler/consistent56/ConsistentParentBasedSampler.java

@@ -27,19 +35,40 @@ final class ConsistentParentBasedSampler extends ConsistentSampler {
   *
   * @param rootSampler the root sampler
   */
-  ConsistentParentBasedSampler(ConsistentSampler rootSampler) {
+  ConsistentParentBasedSampler(ComposableSampler rootSampler) {


Likewise, the OTel ParentBased sampler should be updated to this behavior, I think.

jmacd · 2024-09-06T21:44:29Z

...c/main/java/io/opentelemetry/contrib/sampler/consistent56/ConsistentRateLimitingSampler.java

+          oldState.effectiveWindowCount + 1,
+          oldState.effectiveWindowNanos,
+          oldState.lastNanoTime,
+          oldState.effectiveDelegateProbability);


I think a comment will help -- what's this branch doing?

I'll revisit this part, and add a unit test to cover it.

jmacd · 2024-09-06T21:50:05Z

...nt-sampling/src/test/java/io/opentelemetry/contrib/sampler/consistent56/CoinFlipSampler.java

+ * of its two delegates. Used by unit tests.
+ */
+@Immutable
+final class CoinFlipSampler extends ConsistentSampler {


jmacd · 2024-09-06T21:50:27Z

...ent-sampling/src/test/java/io/opentelemetry/contrib/sampler/consistent56/MarkingSampler.java

+ * could be also offered as a general utility.
+ */
+@Immutable
+final class MarkingSampler implements ComposableSampler {


Co-authored-by: Otmar Ertl <[email protected]>

commit b84a73e Merge: 352202f 7df9862 Author: SylvainJuge <[email protected]> Date: Tue Sep 17 11:13:11 2024 +0200 Merge pull request #2 from SylvainJuge/jmx-scraper-it basic JMX client implementation + tests commit 7df9862 Author: Sylvain Juge <[email protected]> Date: Tue Sep 17 11:11:50 2024 +0200 fix bad merge again commit e4a8f83 Author: Sylvain Juge <[email protected]> Date: Tue Sep 17 11:10:26 2024 +0200 fix bad merge again commit dad197b Author: Sylvain Juge <[email protected]> Date: Tue Sep 17 11:09:18 2024 +0200 fix bad merge commit f8a73d7 Author: Sylvain Juge <[email protected]> Date: Tue Sep 17 11:08:42 2024 +0200 spotless commit eab01e9 Author: Sylvain Juge <[email protected]> Date: Tue Sep 17 11:01:47 2024 +0200 spotless commit f6667dd Merge: 5b3ef9d 352202f Author: Sylvain Juge <[email protected]> Date: Tue Sep 17 10:55:34 2024 +0200 Merge branch 'jmx-scrapper' of github.com:SylvainJuge/opentelemetry-java-contrib into jmx-scraper-it commit 5b3ef9d Author: Sylvain Juge <[email protected]> Date: Tue Sep 17 10:51:11 2024 +0200 add TODOs commit 352202f Merge: 5a82aac 40a2591 Author: SylvainJuge <[email protected]> Date: Tue Sep 17 10:50:50 2024 +0200 Merge pull request #1 from robsunday/jmx-scrapper Initial commit of JmxScraper code. commit 40a2591 Author: jack-berg <[email protected]> Date: Mon Sep 16 16:05:50 2024 -0500 Add declarative config support for RuleBasedRoutingSampler (open-telemetry#1440) commit 6d39e93 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Mon Sep 16 10:40:16 2024 -0700 Update dependency com.uber.nullaway:nullaway to v0.11.3 (open-telemetry#1457) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit eb5dc6f Author: Bruno Baptista <[email protected]> Date: Mon Sep 16 18:26:53 2024 +0100 Fix native mode error cause by static init of random (open-telemetry#862) commit 5ffa348 Author: jack-berg <[email protected]> Date: Mon Sep 16 12:23:41 2024 -0500 Add declarative config support for aws xray propagators (open-telemetry#1442) commit 151b744 Author: SylvainJuge <[email protected]> Date: Mon Sep 16 19:23:19 2024 +0200 add span stacktrace config option (open-telemetry#1414) Co-authored-by: jackshirazi <[email protected]> commit 9ee5fb1 Author: Yury Bubnov <[email protected]> Date: Mon Sep 16 10:18:51 2024 -0700 Issue-1034 Short XRay Trace (open-telemetry#1036) commit 11a2e1a Author: Jeffrey Chien <[email protected]> Date: Mon Sep 16 13:18:10 2024 -0400 Fix Tomcat metric definitions to aggregate multiple MBeans. (open-telemetry#1366) commit 9fa44be Author: Pranav Sharma <[email protected]> Date: Mon Sep 16 11:22:32 2024 -0400 Fix incorrect cloud.platform value for GCF (open-telemetry#1454) commit 11f2111 Author: Peter Findeisen <[email protected]> Date: Mon Sep 16 08:21:46 2024 -0700 Composite Samplers prototype (open-telemetry#1443) Co-authored-by: Otmar Ertl <[email protected]> commit b3386de Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Mon Sep 16 08:20:44 2024 -0700 Update plugin com.squareup.wire to v5.1.0 (open-telemetry#1452) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit 27b631d Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Mon Sep 16 08:20:21 2024 -0700 Update errorProneVersion to v2.32.0 (open-telemetry#1453) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit 791df4b Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Mon Sep 16 09:50:03 2024 -0500 Update dependency io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha to v2.8.0-alpha (open-telemetry#1456) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Jack Berg <[email protected]> commit 19357e6 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Tue Sep 10 15:20:46 2024 -0700 Update dependency com.gradle.enterprise:com.gradle.enterprise.gradle.plugin to v3.18.1 (open-telemetry#1450) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit 460470b Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Tue Sep 10 15:20:23 2024 -0700 Update plugin com.gradle.develocity to v3.18.1 (open-telemetry#1451) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit b45cdab Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Tue Sep 10 12:32:56 2024 +0300 Update dependency com.linecorp.armeria:armeria-bom to v1.30.1 (open-telemetry#1449) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit d36106e Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Tue Sep 10 09:11:26 2024 +0300 Update micrometer packages to v1.13.4 (open-telemetry#1448) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit fb25c79 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Mon Sep 9 15:44:53 2024 +0300 Update dependency gradle to v8.10.1 (open-telemetry#1447) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit f847561 Author: robsunday <[email protected]> Date: Tue Sep 17 09:22:21 2024 +0200 Code review changes: Argument parsing moved to the main class. Argument validation now throw exception. More tests added for config factory. Created unit tests for JmxScraper class. commit 03788ff Author: Sylvain Juge <[email protected]> Date: Mon Sep 16 17:26:12 2024 +0200 add TODO for testing server SSL support commit a3fbeb5 Author: Sylvain Juge <[email protected]> Date: Mon Sep 16 17:08:16 2024 +0200 add TODO to enable server-side SSL commit 1619595 Author: Sylvain Juge <[email protected]> Date: Mon Sep 16 16:59:17 2024 +0200 wip commit 164679f Author: robsunday <[email protected]> Date: Wed Sep 11 12:34:12 2024 +0200 Cleanup of config. Refactoring of JmxScraperConfigFactory. Added first unit tests. commit 165fcb8 Author: robsunday <[email protected]> Date: Tue Sep 10 12:36:29 2024 +0200 Initial commit of JmxScraper code. Application compiles and runs as standalone and can read config file.

Composite Samplers prototype

f242459

This PR modifies the Consistent Probability Samplers to make them conform to the proposed Composite Samplers at open-telemetry/oteps#250

PeterF778 requested a review from a team August 30, 2024 00:29

github-actions bot assigned oertl and PeterF778 Aug 30, 2024

github-actions bot requested a review from oertl August 30, 2024 00:29

PeterF778 marked this pull request as draft August 30, 2024 14:43

PeterF778 added 2 commits August 30, 2024 14:53

Refactoring.

23690a4

Refactoring, adding a new test case based on open-telemetry/oteps#250

a8443c4

PeterF778 marked this pull request as ready for review September 3, 2024 21:07

oertl reviewed Sep 5, 2024

View reviewed changes

PeterF778 and others added 14 commits September 5, 2024 15:02

Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…

9b53416

…pler/consistent56/ComposableSampler.java Co-authored-by: Otmar Ertl <[email protected]>

Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…

b3b57ac

…pler/consistent56/SamplingIntent.java Co-authored-by: Otmar Ertl <[email protected]>

Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…

7c13e4a

…pler/consistent56/SamplingIntent.java Co-authored-by: Otmar Ertl <[email protected]>

Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…

3308d4a

…pler/consistent56/SamplingIntent.java Co-authored-by: Otmar Ertl <[email protected]>

Update consistent-sampling/src/test/java/io/opentelemetry/contrib/sam…

49fd364

…pler/consistent56/UseCaseTest.java Co-authored-by: Otmar Ertl <[email protected]>

Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…

7db19ea

…pler/consistent56/ConsistentRuleBasedSampler.java Co-authored-by: Otmar Ertl <[email protected]>

Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…

75201a9

…pler/consistent56/ConsistentAnyOf.java Co-authored-by: Otmar Ertl <[email protected]>

Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…

e017c45

…pler/consistent56/ConsistentRuleBasedSampler.java Co-authored-by: Otmar Ertl <[email protected]>

Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…

02c3411

…pler/consistent56/ConsistentRuleBasedSampler.java Co-authored-by: Otmar Ertl <[email protected]>

Update consistent-sampling/src/test/java/io/opentelemetry/contrib/sam…

66cf1d7

…pler/consistent56/CoinFlipSampler.java Co-authored-by: Otmar Ertl <[email protected]>

Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…

9a21ab1

…pler/consistent56/ConsistentAnyOf.java Co-authored-by: Otmar Ertl <[email protected]>

Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…

1d0ba27

…pler/consistent56/ConsistentAnyOf.java Co-authored-by: Otmar Ertl <[email protected]>

Addressing code review suggestions.

eb66327

Generalizing CoinFlipSampler.

03104e7

jmacd reviewed Sep 6, 2024

View reviewed changes

jmacd approved these changes Sep 6, 2024

View reviewed changes

Adding a test for low-resolution clock.

17aa757

oertl approved these changes Sep 16, 2024

View reviewed changes

trask approved these changes Sep 16, 2024

View reviewed changes

trask merged commit 711f617 into open-telemetry:main Sep 16, 2024
14 checks passed

robsunday pushed a commit to robsunday/opentelemetry-java-contrib that referenced this pull request Sep 17, 2024

Composite Samplers prototype (open-telemetry#1443)

11f2111

Co-authored-by: Otmar Ertl <[email protected]>

This was referenced Sep 18, 2024

Bootstrap integration tests SylvainJuge/opentelemetry-java-contrib#4

Merged

prepare PR to be merged SylvainJuge/opentelemetry-java-contrib#5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Composite Samplers prototype #1443

Composite Samplers prototype #1443

PeterF778 commented Aug 30, 2024

jmacd Sep 6, 2024

jmacd Sep 6, 2024

PeterF778 Sep 10, 2024

jmacd Sep 6, 2024

jmacd Sep 6, 2024

PeterF778 Sep 10, 2024

jmacd Sep 6, 2024

jmacd Sep 6, 2024

Composite Samplers prototype #1443

Composite Samplers prototype #1443

Conversation

PeterF778 commented Aug 30, 2024

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