Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unreasonable restriction on Sampler's description to be immutable #2095

Closed
yurishkuro opened this issue Nov 2, 2021 · 3 comments · Fixed by #4137
Closed

Remove unreasonable restriction on Sampler's description to be immutable #2095

yurishkuro opened this issue Nov 2, 2021 · 3 comments · Fixed by #4137
Assignees
Labels
spec:trace Related to the specification/trace directory triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Nov 2, 2021

What are you trying to achieve?

open-telemetry/opentelemetry-go-contrib#936 (comment)

The spec current says:

Description MUST NOT change over time and caller can cache the returned value.

What did you expect to see?

This is unreasonable requirement. First, who is the "caller" who requires this? Second, if this is for efficiency purposes, the caching can be done by the sampler internally when possible. Finally, this restriction fundamentally does not work with samplers that update their sampling policy based on remote control plane.

Proposal

Remove this line from the spec.

@yurishkuro yurishkuro added the spec:trace Related to the specification/trace directory label Nov 2, 2021
yvrhdn pushed a commit to yvrhdn/opentelemetry-go-contrib that referenced this issue Nov 8, 2021
PR to update spec: open-telemetry/opentelemetry-specification#2095
If this is approved we can revert this commit.
@jmacd
Copy link
Contributor

jmacd commented Nov 9, 2021

I have questions about the arguments made.

First, who is the "caller" who requires this?

Who is the "caller" who disagrees? I'm interested because in #2047 I have written about what sort of trace completeness guarantees we may expect from a sampler, and the conclusion is that the consumer of a trace ought to know the minimum sampling probability used across the trace. The expectation is that the producer is somehow able to convey this information to the consumer, but we aren't specifying how.

Using the Description() would be one way to learn this information, but it doesn't seem reliable because I would have to probe it very frequently. If you want to specify a mechanism for a span to remember how it was sampled--perhaps a descriptive string--shouldn't that become a span attribute? Then we'll all get something well defined.

Second, if this is for efficiency purposes, the caching can be done by the sampler internally when possible.

How would a Sampler determine when this is possible? It doesn't sound like something for general use.

Finally, this restriction fundamentally does not work with samplers that update their sampling policy based on remote control plane.

You haven't said what you're trying to make work, but it sounds like a fixed description like jaeger_remote:policy_id won't do. Will you log this information periodically?

@yurishkuro
Copy link
Member Author

Who is the "caller" who disagrees?

How is that relevant? The requirement is onerous on the provider (sampler), not on the caller, so the caller needs to provide strong reasons for it.

If you want to specify a mechanism for a span to remember how it was sampled--perhaps a descriptive string--shouldn't that become a span attribute?

100% agree. I think Description has nothing to do with this use case, thus my question. who is the caller who must live or die by this description string being immutable? I see description as purely a debugging / introspection tool, to answer questions like "what's my sampling rate right now?", e.g. from z-pages.

How would a Sampler determine when this is possible? It doesn't sound like something for general use.

Trivially. A sampler whose parameters never change after instantiation can prep the description string once and always return it (iirc some Jaeger samplers did that). A sampler whose parameters can change via control plane can still cache the string upon accepting those change. A sampler whose parameters change continuously cannot satisfy the immutability requirement at all.

You haven't said what you're trying to make work, but it sounds like a fixed description like jaeger_remote:policy_id won't do.

I believe the intention of the description string was to be reflective of the current parameters. If we follow your suggestion, then the string will be simply jaeger_remote:<service_id> which is not especially helpful as introspection tool. All examples in the spec for other samplers show strings that contain both sampler name and its parameters.

Aneurysm9 added a commit to open-telemetry/opentelemetry-go-contrib that referenced this issue Mar 11, 2022
* Add Jaeger remote sampler

* add jaeger_remote/example

* Extract samplingStrategyParser

* Generate code from jaeger-idl

* Add per operation sampler, fix CI

* Fix Description()

* Delete jaeger-idl submodule, directly check in sampling.proto

* Update OTel dependencies

* Update README.md

* Improve test coverage

* (Sampler).Description() must not change over time

PR to update spec: open-telemetry/opentelemetry-specification#2095
If this is approved we can revert this commit.

* Replace internals with jaeger-client-go

* Address linting issues

* Fix race condition in test

* Update samplers/jaegerremote/README.md

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

* Update samplers/jaegerremote/internal/testutils/mock_agent.go

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

* revmoe go.mod debug info

* fix Copyright and

* add from

* Update dependency

* Modified according to pr

* change config from https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md#configuration

* (fix:sampler_remote_options.go):change config

* (fix:constants.go):remove Copyright

* (fix:samoler_remote.go):fix config and add default

* Update samplers/jaegerremote/sampler_remote_options.go

yet

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

* add local path

* fix lint

* make precommit fix

* (fix:sampler_remote_option.go):The default port of jaeger agent is 5778, but there are other ports specified by the user, so the sampling address and fetch address are strongly bound

* change for Style guide

* change for Update samplers/jaegerremote/sampler_remote_options.go

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: dino.ma <dino_ma@163.com>
@yurishkuro
Copy link
Member Author

Since I didn't see well-argued objections -- #4137

@danielgblanco danielgblanco added the triage:accepted:ready Ready to be implemented. Small enough or uncontroversial enough to be implemented without sponsor label Jul 15, 2024
@austinlparker austinlparker moved this to Spec - Accepted in 🔭 Main Backlog Jul 16, 2024
@trask trask assigned yurishkuro and unassigned jmacd Jul 16, 2024
@trask trask added triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned and removed triage:accepted:ready Ready to be implemented. Small enough or uncontroversial enough to be implemented without sponsor labels Jul 16, 2024
@trask trask moved this from Spec - Accepted to Spec - In Progress in 🔭 Main Backlog Jul 16, 2024
@danielgblanco danielgblanco moved this from Spec - In Progress to Spec - Closed in 🔭 Main Backlog Jul 29, 2024
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 31, 2024
…try#4137)

Fixes open-telemetry#2095

## Changes

State that sampler description can change over time. Provide examples.

---------

Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Robert Pająk <pellared@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:trace Related to the specification/trace directory triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned
Projects
Status: Spec - Closed
Development

Successfully merging a pull request may close this issue.

4 participants