Skip to content

[processor/dynamic_sampling] move buffered spans at decision time instead of copying - #50044

Merged
songy23 merged 1 commit into
open-telemetry:mainfrom
MikeGoldsmith:mike/dynamicsampling-decide-moveto
Aug 4, 2026
Merged

[processor/dynamic_sampling] move buffered spans at decision time instead of copying#50044
songy23 merged 1 commit into
open-telemetry:mainfrom
MikeGoldsmith:mike/dynamicsampling-decide-moveto

Conversation

@MikeGoldsmith

@MikeGoldsmith MikeGoldsmith commented Aug 4, 2026

Copy link
Copy Markdown
Member

Description

Companion to #50026, applied to the decision path. assembleTrace deep-copied every buffered ResourceSpans into the output at decision time, and profiling showed that copy was ~52% of decision-path allocation bytes. The buffered spans are processor-owned (created fresh in ConsumeTraces) and the pending trace is discarded after the decision, so they can be moved instead. readIncomingSampling reads the same spans and already runs before assembly; a comment now pins that ordering and finishDecision nils the consumed slice.

Also fixes two benchmark issues found while measuring:

  • The incoming-tracestate decide benchmark used an rv of 16 hex digits (the spec requires exactly 14), so it exercised the parse-failure path and understated real tracestate cost.
  • BenchmarkDecide reused one pendingTrace across iterations, which breaks once spans are consumed; it now rebuilds the trace per iteration (untimed).

Benchmarks (Apple M4 Pro, mean of 5 runs, measured after the benchmark fixes so before/after are like-for-like):

BenchmarkDecide case ns/op before ns/op after Δ B/op before B/op after Δ allocs/op before allocs/op after Δ
1span_catchall 1,931 1,436 -26% 1,272 648 -49% 24 13 -46%
10spans_catchall 4,523 3,102 -31% 7,392 3,320 -55% 105 49 -53%
10spans_catchall_incoming_tracestate 7,852 6,721 -14% 8,272 4,200 -49% 135 79 -41%
10spans_5ottl_rules 6,677 5,424 -19% 8,042 3,967 -51% 145 89 -39%
100spans_catchall 27,739 16,044 -42% 68,688 29,960 -56% 915 409 -55%
100spans_5ottl_rules 44,015 34,277 -22% 75,174 36,416 -52% 1,315 809 -38%
1000spans_catchall 264,149 150,837 -43% 680,791 296,364 -56% 9,015 4,009 -56%
10000spans_catchall 2,435,400 1,163,242 -52% 6,802,536 2,960,366 -56% 90,015 40,009 -56%
10000spans_5ottl_rules 4,084,604 2,673,498 -35% 7,444,723 3,602,399 -52% 130,018 80,011 -38%

Link to tracking issue

Refs #49311

Testing

  • Full suite passes with the race detector; existing decide/eviction/late-span tests cover the consumed-spans semantics.
  • Before/after benchmark comparison across all BenchmarkDecide cases (numbers above).

Documentation

Comments on assembleTrace and finishDecision documenting the move semantics and ordering constraint. Changelog entry included.

Authorship

  • I, a human, wrote this pull request description myself.

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Aug 4, 2026

Copy link
Copy Markdown

Pull request dashboard status

Merged · refreshed 2026-08-04 19:31 UTC

Status above doesn't look right?
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

@songy23
songy23 merged commit cf487ec into open-telemetry:main Aug 4, 2026
139 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants