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

Tracer must expose getter for sampler #125

Closed
SergeyKanzhelev opened this issue Jun 14, 2019 · 7 comments
Closed

Tracer must expose getter for sampler #125

SergeyKanzhelev opened this issue Jun 14, 2019 · 7 comments
Labels
area:api Cross language API specification issue area:sampling Related to trace sampling

Comments

@SergeyKanzhelev
Copy link
Member

Sampler access is needed if you are using recordSpanData and still want to apply sampling

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

jmacd commented Jun 17, 2019

I can't be certain from this description, but as issue 71 lays out, I suspect would not be needed if we removed recordSpanData from the user-facing API. Could you clarify?

@SergeyKanzhelev
Copy link
Member Author

@jmacd if API will be removed - there may not be a need for sampler. Right now the API is a bad place where scenario of reporting out-of-band spans using the same sampling algo is not possible.

@jmacd
Copy link
Contributor

jmacd commented Jun 17, 2019

I can't understand this claim, because I still haven't see a formal argument why two APIs are required to support out-of-band reporting.

@SergeyKanzhelev
Copy link
Member Author

@jmacd we discussed it a few times already. Initial agreement is formulated in Java API. And I'm saying it is not complete. Either sampler needs to be exposed or API needs to be changed. Thus the issue

@bogdandrutu
Copy link
Member

I think this depends on the issue (which unfortunately I cannot find anymore) about keeping SpanData vs offering APIs to allow start/end timestamps.

@jmacd
Copy link
Contributor

jmacd commented Jul 1, 2019

I've been digging into the SpanData API for a while now, and have teased apart several requirements it seems. They are:

(1) support recording spans with alternate resources ("out of band")
(2) support recording spans with alternate timestamps ("asynchronous")
(3) support recording lazy spans

I was confused at why the topic of the Sampler had entered the discussion about SpanData, but now I think I see it. Is there a requirement here that lazy-span reporting interact directly with the Sampler?

I.e., are we requiring (in the current spec) that a call to recordSpanData, with complete details including start and end timestamps plus accumulated span attributes, that the Sampler SHOULD use those details?

See also issue 164. I can't quite figure out why the Sampler is considered a user-facing API, vs. part of the SDK.

@iredelmeier iredelmeier added the area:sampling Related to trace sampling label Jul 30, 2019
@SergeyKanzhelev SergeyKanzhelev removed this from the API revision: 07-2019 milestone Sep 27, 2019
@SergeyKanzhelev
Copy link
Member Author

SpanData API was removed. Closing the issue

TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this issue Oct 15, 2020
rockb1017 pushed a commit to rockb1017/opentelemetry-specification that referenced this issue Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue area:sampling Related to trace sampling
Projects
None yet
Development

No branches or pull requests

4 participants