Add incubator implementation of composite sampling spec by anuraaga · Pull Request #7626

Add incubator implementation of composite sampling spec by anuraaga · Pull Request #7626 · open-telemetry/opentelemetry-java

		Attributes attributes,
		List<LinkData> parentLinks) {
		TraceState traceState = Span.fromContext(parentContext).getSpanContext().getTraceState();
		OtelTraceState otTraceState = OtelTraceState.parse(traceState);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

teency nit: rename to otelTraceState so as to not confuse my poor brain between this and openTracing.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought by now we may have been past that but guess not ;)

		otTraceState.getRandomValue(), INVALID_THRESHOLD, otTraceState.getRest());
		}

		String ot = otTraceState.serialize();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename to "seralizedState"?

Seems like this would be an excellent target for some property-based tests. I can't remember if we have any in here already, but it might be a good opportunity, since we can very clearly define the invariants, and generate random inputs, verifying the outputs match the expectations.

Thanks - we have fuzz tests already which I think are basically what you're getting at. Sounds good will add some

Thanks - we have fuzz tests already which I think are basically what you're getting at. Sounds good will add some

Fuzz testing is slightly different, in that the goal is usually just to make sure that things don't break, rather than testing invariants, but if you can use the same framework to accomplish property testing, that would be awesome.

		boolean sampled = false;
		if (isValidThreshold(intent.getThreshold())) {
		thresholdReliable = intent.isThresholdReliable();
		long randomValue;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the threshold is not reliable, the sampler must not use the random value from the trace state or from the trace-id. This is because we cannot guarantee that these "random" values have the required uniform distribution over the (MIN_THRESHOLD, MAX_THRESHOLD) interval.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find anything in the spec on this. I noticed the caveat to deal with rate limiting sampler but since that's also not in the spec went with the simpler implementation. Is this something to change now or can we follow up after spec clarification?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok went ahead and switched to generating a random when unreliable

		boolean sampled = false;
		if (isValidThreshold(intent.getThreshold())) {
		thresholdReliable = intent.isThresholdReliable();
		long randomValue;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok went ahead and switched to generating a random when unreliable

		Attributes attributes,
		List<LinkData> parentLinks) {
		TraceState traceState = Span.fromContext(parentContext).getSpanContext().getTraceState();
		OtelTraceState otTraceState = OtelTraceState.parse(traceState);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought by now we may have been past that but guess not ;)

		assertState(output, rv, th);
		}

		private static void assertState(OtelTraceState state, long rv, long th) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkwatson This is what I came up with for testing properties based on fuzz. What do you think?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say I grok all the assertions, but this looks good! Thanks!

@jkwatson Think we can proceed with this PR, or anything I should look at? Thanks

@jkwatson Think we can proceed with this PR, or anything I should look at? Thanks

If @PeterF778 can give it a 👍🏽 then I think we're good to go.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice and clean, thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Thank you for your contribution @anuraaga! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

the-clam pushed a commit to the-clam/opentelemetry-java that referenced this pull request

Nov 6, 2025