feat: add sampling attributes for jaeger remote sampler #8073

etilite · 2025-10-26T19:07:19Z

Which problem is this PR solving?

Resolves #3653
Resolves #691

Description of the changes

Added option WithAttributesOn which adds jaeger.sampler.param and jaeger.sampler.type attributes for sampled spans

linux-foundation-easycla · 2025-10-26T19:07:27Z

The committers listed above are authorized under a signed CLA.

✅ login: etilite / name: etilite (b7591f4, 6cccfd9, dbcae9e, e1a3370, e8aa3fb)

flc1125 · 2025-10-27T00:32:52Z

cc code owner: @yurishkuro PTAL

dmathieu

Those attributes don't match anything defined in semantic conventions.

yurishkuro

I suggest writing a test in a different package like jaegerremote_test, to make sure you got the visibility of new functions / types correctly

samplers/jaegerremote/sampler_remote_options.go

yurishkuro · 2025-10-27T13:53:33Z

samplers/jaegerremote/sampler.go

+
+type samplerOptionFunc func(*samplerOptions)
+
+func withAttributesOn() samplerOptionFunc {


I would make then on by default, and have this option to turn off. The function needs to be public in order to be used from a different package.

Suggested change

func withAttributesOn() samplerOptionFunc {

func WithAttributesDisabled() samplerOptionFunc {

I wasn't sure that we want to change current default behavior - jaegerremote does not set any attributes.

This is supposed to be an internal option for creating and updating samplers which constructors also aren't exported.

For external usage I added WithAttributesOn option.

I do want to change the current behavior. The current behavior is useless, the sampler does not actually work with Jaeger's adaptive backend. And the change itself is additive, it's backwards compatible (especially since the new attributes are prefixed with jaeger.).

do we need an option to disable these attributes in that case? If not implementation will be simple and no need for internal sampler options too

Personally I would argue you don't even need the opt-out option, OTEL SDK is already extremely verbose in terms of how many attributes it captures, having two extra ones on root span only seems irrelevant. But for 100% backwards compat you would provide an option to opt-out.

yurishkuro · 2025-10-27T13:55:58Z

samplers/jaegerremote/sampler.go

 	samplingRate float64
 	sampler      trace.Sampler
+	attributes   []attribute.KeyValue
+	attributesOn bool


I think this flag is only used at initialization, you don't really need to store it

When probabilisticSampler or ratelimitngSampler are being updated its init method is also called, so I not sure if we can omit this flag.

yurishkuro · 2025-10-27T13:59:20Z

samplers/jaegerremote/sampler.go

+	samplerTypeKey                = "sampler.type"
+	samplerTypeValueProbabilistic = "probabilistic"
+	samplerTypeValueRateLimiting  = "ratelimiting"
+	samplerParamKey               = "sampler.param"


I think it would make sense to prefix with jaeger., This may require small change in the backend to recognize, but it separates custom attributes into clear namespace.

Suggested change

samplerTypeKey = "sampler.type"

samplerTypeValueProbabilistic = "probabilistic"

samplerTypeValueRateLimiting = "ratelimiting"

samplerParamKey = "sampler.param"

samplerTypeKey = "jaeger.sampler.type"

samplerParamKey = "jaeger.sampler.param"

samplerTypeValueProbabilistic = "probabilistic"

samplerTypeValueRateLimiting = "ratelimiting"

Yeah, this is reasonable but I see some difficulties when using in mixed environments (jaeger and otel services) where will be different sampler attributes in that case. Not sure if backend can support more that one key.

this is a Jaeger-specific functionality and a Jaeger-specific sampler. Since it's not setting any attributes now then nothing is depending on them.

So can we keep canonical keys? I did small research and using new keys will require some overhead on collector to transform these attributes.

Since those are jaeger specific, prefixing them with jaeger definitely makes sense.

Added prefix

yurishkuro · 2025-10-27T14:00:39Z

Thank you for this PR! Originally when I looked at this problem there was no way in the API to pass back attributes (I think I even had an option issue about that).

etilite · 2025-10-27T19:13:15Z

Those attributes don't match anything defined in semantic conventions.

Can we do anything about it? Those attributes provide back-compatibility with jaeger-remote-sampler and can be turned only by an option. So yeah it is kinda jaeger attributes.

Wrong assessment.

etilite · 2025-10-28T23:21:51Z

@dmathieu @yurishkuro If you’re okay with it, I could try a different approach - add two new options - functions that create attributes for probabilistic/ratelimiting samplers, so user can define their own attributes to solve this issue.
This solution does not interfere with the semantic convention.

dmathieu · 2025-10-29T08:49:24Z

Sorry, my assessment was wrong. As these are jaeger specific, they shouldn't need semantic conventions.

codecov · 2025-11-02T16:16:38Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.8%. Comparing base (6342fe6) to head (6cccfd9).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #8073     +/-   ##
=======================================
+ Coverage   80.9%   81.8%   +0.8%     
=======================================
  Files        194     194             
  Lines      13073   13380    +307     
=======================================
+ Hits       10588   10949    +361     
+ Misses      2107    2033     -74     
- Partials     378     398     +20

Files with missing lines	Coverage Δ
samplers/jaegerremote/sampler.go	`92.0% <100.0%> (+1.0%)`	⬆️
samplers/jaegerremote/sampler_remote.go	`90.4% <100.0%> (ø)`
samplers/jaegerremote/sampler_remote_options.go	`98.0% <100.0%> (+<0.1%)`	⬆️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

etilite · 2025-11-02T16:35:38Z

@yurishkuro I added new test in different package and answered on all comments. Review pls

flc1125 · 2025-11-03T01:00:22Z

@yurishkuro I added new test in different package and answered on all comments. Review pls

We can fix the CI errors before entering the review to reduce some trivial checks. 😁

…egerremote-attributes

flc1125 · 2025-11-03T13:52:18Z

@etilite please add a CHANGELOG

flc1125 · 2025-11-04T00:36:43Z

@etilite There are currently some conflicts; please fix them.

etilite · 2025-11-04T09:57:05Z

@flc1125 resolved conflicts but now ci/cd failing. Probably need just to retry these jobs, can't do it myself.

yurishkuro · 2025-11-06T19:32:59Z

samplers/jaegerremote/sampler.go

+	samplerTypeValueRateLimiting  = "ratelimiting"
+)
+
+type samplerOptions struct {


this is redundant. The package already has the functional options mechanism

func New( serviceName string, opts ...Option, ) *Sampler { options := newConfig(opts...)

So WithAttributesOn() should be implemented as Option just like other options, and should set a flag on the config struct.

the idea behind these internal options is to have different behavior when creating samplers without need to change every test to add new argument in constructor.
I can use flag arg in sampler's constructors if it is ok with you.

without need to change every test to add new argument in constructor.

a) why do you need to change every test? do they all compare attributes?
b) even if you need to, it seems like a trivial task for a Copilot

a) cause a lot of tests call these constructors
b) yeah but it is harder to review a lot of small changes
nvm I'll change to flag argument 👍

yurishkuro · 2025-11-06T19:34:52Z

samplers/jaegerremote/sampler.go

+
+type samplerOptionFunc func(*samplerOptions)
+
+func withAttributesOn() samplerOptionFunc {


I do want to change the current behavior. The current behavior is useless, the sampler does not actually work with Jaeger's adaptive backend. And the change itself is additive, it's backwards compatible (especially since the new attributes are prefixed with jaeger.).

yurishkuro · 2025-11-06T23:27:09Z

samplers/jaegerremote/sampler_remote.go

 // Sampler invokes the updaters while holding a lock on the main sampler.
 type samplerUpdater interface {
-	Update(sampler trace.Sampler, strategy any) (modified trace.Sampler, err error)
+	Update(sampler trace.Sampler, strategy any, attributesDisabled bool) (modified trace.Sampler, err error)


It is not necessary to change this interface to pass attributesDisabled. When individual updaters are created the value of this flag is known and can be captured in the updaters themselves. This way we separate configuration information from the execution information (only sampler and strategy are variable at runtime)

Good point! Can I remove withUpdaters option then? Cause it makes harder to properly create updaters and pass attributesDisabled to them. Anyway withUpdaters is not needed for tests now (can be safely removed) and seems to be an old arfifact

withUpdaters is private, safe to remove if not needed. These APIs used to be public for extensibility.

feat: add sampling attributes for jaeger remote sampler

e70276c

etilite requested a review from a team as a code owner October 26, 2025 19:07

github-actions bot requested a review from yurishkuro October 26, 2025 19:07

dmathieu previously requested changes Oct 27, 2025

View reviewed changes

yurishkuro reviewed Oct 27, 2025

View reviewed changes

chore: jaegerremote: add test in separate pkg

f53b5f8

chore: jaegerremote: fix linter

9cc7fce

etilite added 3 commits November 3, 2025 15:38

feat: add sampling attributes for jaeger remote sampler

76fc443

chore: jaegerremote: add test in separate pkg

e2ce983

chore: jaegerremote: fix linter

8e6fd09

etilite force-pushed the jaegerremote-attributes branch from 9cc7fce to 8e6fd09 Compare November 3, 2025 12:38

etilite added 2 commits November 3, 2025 15:58

Merge remote-tracking branch 'origin/jaegerremote-attributes' into ja…

c110c3e

…egerremote-attributes

chore: jaegerremote: move test to separate pkg

65b84a2

yurishkuro approved these changes Nov 3, 2025

View reviewed changes

chore: jaegerremote: add CHANGELOG.md

b862da3

flc1125 approved these changes Nov 4, 2025

View reviewed changes

Merge branch 'main' into jaegerremote-attributes

b7591f4

chore: jaegerremote: add prefix for attribute keys

e1a3370

yurishkuro suggested changes Nov 6, 2025

View reviewed changes

chore: jaegerremote: set attributes by default

e8aa3fb

yurishkuro reviewed Nov 6, 2025

View reviewed changes

etilite added 2 commits November 7, 2025 22:24

chore: jaegerremote: rm redundant internal option withUpdaters

dbcae9e

chore: jaegerremote: add attributesDisabled for updaters

6cccfd9

yurishkuro approved these changes Nov 7, 2025

View reviewed changes


		type samplerOptionFunc func(*samplerOptions)

		func withAttributesOn() samplerOptionFunc {

	func withAttributesOn() samplerOptionFunc {
	func WithAttributesDisabled() samplerOptionFunc {

feat: add sampling attributes for jaeger remote sampler #8073

Are you sure you want to change the base?

feat: add sampling attributes for jaeger remote sampler #8073

Conversation

etilite commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Description of the changes

Uh oh!

linux-foundation-easycla bot commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flc1125 commented Oct 27, 2025

Uh oh!

dmathieu left a comment

Choose a reason for hiding this comment

Uh oh!

yurishkuro 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!

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!

Choose a reason for hiding this comment

Uh oh!

yurishkuro commented Oct 27, 2025

Uh oh!

etilite commented Oct 27, 2025

Uh oh!

etilite commented Oct 28, 2025

Uh oh!

dmathieu commented Oct 29, 2025

Uh oh!

codecov bot commented Nov 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

etilite commented Nov 2, 2025

Uh oh!

flc1125 commented Nov 3, 2025

Uh oh!

flc1125 commented Nov 3, 2025

Uh oh!

flc1125 commented Nov 4, 2025

Uh oh!

etilite commented Nov 4, 2025

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!

etilite Nov 6, 2025 • 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

etilite commented Oct 26, 2025 •

edited

Loading

linux-foundation-easycla bot commented Oct 26, 2025 •

edited

Loading

codecov bot commented Nov 2, 2025 •

edited

Loading

etilite Nov 6, 2025 •

edited

Loading

etilite Nov 6, 2025 •

edited

Loading