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 hasRemoteParent from Sampler.shouldSample method #164

Closed
pavolloffay opened this issue Jun 26, 2019 · 6 comments
Closed

Remove hasRemoteParent from Sampler.shouldSample method #164

pavolloffay opened this issue Jun 26, 2019 · 6 comments
Assignees
Labels
area:sdk Related to the SDK
Milestone

Comments

@pavolloffay
Copy link
Member

Created from open-telemetry/opentelemetry-java#430 (comment)

I think we should remove this for the moment because no clear use for this. I think what we should add support later is a notion of "untrusted parent". But probably that needs a RFC. So I would suggest we file an issue on the specification to remove this.

https:/open-telemetry/opentelemetry-java/blob/ed42237b6c71052bbffe4d849647c53fae81b246/api/src/main/java/io/opentelemetry/trace/Sampler.java#L44-L50

cc @bogdandrutu

@bogdandrutu
Copy link
Member

To clarify a bit the comment: I am proposing to remove the hasRemoteParent from the Sampler interface because with the latest changes in the SpanBuilder, a child Span can be started with a SpanContext as parent even if the parent is not remote, so we cannot determine correctly if the parent is remote or not.

@bogdandrutu bogdandrutu added the area:api Cross language API specification issue label Jun 26, 2019
@bogdandrutu bogdandrutu added this to the API revision: 07-2019 milestone Jun 26, 2019
@jmacd
Copy link
Contributor

jmacd commented Jul 1, 2019

I admit lacking some of the details that went into the current Java prototype, but on the topic of Sampler API, I have questions. Why is this considered area:API? I see this as an SDK interface. The SDK supports an API for sampling, but it is distinct from the end-user instrumentation API.

Rationale: a user never interacts directly with the Sampler.

Should this be classified area:SDK?

@bogdandrutu
Copy link
Member

@jmacd it cannot be in the SDK because users need this when instrumenting their applications. Imagine using the OpenTelemetry with an implementation that does upfront sampling and for some specific requests users always want to be sampled. If the Sampler is in the SDK they need to depend on the SDK to be able to specify that for this particular request/Span I always/never want sampling.

I think this behavior should be supported in the API to avoid users start depending on a specific implementation.

@z1c0
Copy link
Contributor

z1c0 commented Jul 9, 2019

For our sampling implementation, a fine-grained sampling API would not be a good fit.
An opinionated sampler implementation should be able to make this decision for itself.
A slim API (e.g. a hint or recommendation) would be a good compromise.

@SergeyKanzhelev
Copy link
Member

@bogdandrutu assigning to you as you are working on moving sampling to SDK and related changes

@bogdandrutu bogdandrutu added area:sdk Related to the SDK and removed area:api Cross language API specification issue area:sampling Related to trace sampling labels Oct 9, 2019
@bogdandrutu bogdandrutu modified the milestones: Alpha v0.2, Alpha v0.3 Oct 9, 2019
@jmacd
Copy link
Contributor

jmacd commented Jan 22, 2020

Closing this for now as there are a number of in-flight and unaccepted issues/PRs about sampling. This is stale.

@jmacd jmacd closed this as completed Jan 22, 2020
TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this issue Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK
Projects
None yet
Development

No branches or pull requests

6 participants