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

Clarification on how processors handle sampling decisions. #3779

Open
HaloFour opened this issue Nov 30, 2023 · 8 comments
Open

Clarification on how processors handle sampling decisions. #3779

HaloFour opened this issue Nov 30, 2023 · 8 comments
Assignees
Labels
sig-issue A specific SIG should look into this before discussing at the spec spec:trace Related to the specification/trace directory

Comments

@HaloFour
Copy link

What are you trying to achieve?

I would like to clarify the following statement in the Trace SDK specification:

Span Exporters MUST receive those spans which have Sampled flag set to true and they SHOULD NOT receive the ones that do not.

Particularly around the conversation on this PR here: #871 (comment)

The implication is that it's not forbidden to have a mechanism, even for the built-in processors, to allow unsampled spans to be forwarded to an exporter, as long as it's explicitly an opt-in mechanism. I would like to request such a mechanism be added to the BatchSpanProcessor in the Java SDK support a mechanism to allow unsampled spans to be batched and forwarded to the exporter.

Additional context.

In our case we have a number of services operated by different teams with their own observability stacks. We would like to have the ability to export a substantially higher percentage of spans to our stack without imposing that sampling percentage to our upstream dependencies.

@HaloFour HaloFour added the spec:trace Related to the specification/trace directory label Nov 30, 2023
@tigrannajaryan
Copy link
Member

From what I understand this asks for the spec to specify that the SDK is allowed to have an particular opti-in capability (to export non-sampled spans). I don't think we want to have that type of language in the spec. Extra opti-in functionality does not necessarily have to be explicitly permitted in the spec. If it is not prohibited in the spec then it is permitted implicitly.

@jmacd had thoughts about making the 4th type of decision (non-sampled/exported) a feature. If that happens in the future then it certainly will become part of the spec as a requirement.

@jmacd
Copy link
Contributor

jmacd commented Dec 6, 2023

I believe this is a duplicate of #2986.

Another open issue, #2918, discusses this topic. See #2918 (comment).

Since I see this ultimately as a sampling-related problem, you are invited to join the weekly Sampling SIG, which is Thursdays at 4PM UTC.

@jmacd jmacd closed this as completed Dec 6, 2023
@HaloFour
Copy link
Author

HaloFour commented Dec 6, 2023

I disagree that this is a duplicate. This isn't about changing or expanding upon sampling decisions, it's about allowing a processor to explicitly opt-in to ignoring the sampling decision, or derive it's own.

The spec, as is, appears to afford SDK implementations the wiggle room to do that already, but the Java SDK folks feel uncertain about adding such an option to BatchSpanProcessor given the current verbiage.

@jmacd
Copy link
Contributor

jmacd commented Dec 6, 2023

@HaloFour Thank you. @tigrannajaryan tried to address your request about opt-in behaviors in the default SDKs, which is not explicitly disallowed.

#2986 appears to be a solution-agnostic request for the same functionality, pointing out that we lack a way to export unsampled spans, so duplicate in nature if not phrased exactly the same.

However, I'm coming around to seeing the logic in your request.

Span Exporters MUST receive those spans which have Sampled flag set to true and they SHOULD NOT receive the ones that do not.

This specification is somewhat misplaced. The library guidelines indicate that exporters should do protocol, not logic. It should be the processor's decision to export or not export, and the exporter should not question it.

The batch span processor -- it SHOULD export spans that are sampled and it SHOULD NOT export spans that are not sampled. However, these are "SHOULD" statements and then, I think, you will have more freedom to experiment with opt-in processor behavior. Does that sound good?

@jmacd jmacd reopened this Dec 6, 2023
@jmacd jmacd added the [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR label Dec 6, 2023
@HaloFour
Copy link
Author

HaloFour commented Dec 7, 2023

/cc @jack-berg @jkwatson

Would that change allay concerns about allowing the SDK span processors support an opt-in mechanism to export un-sampled spans?

@austinlparker austinlparker added sig-issue A specific SIG should look into this before discussing at the spec and removed [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR labels Apr 30, 2024
@austinlparker
Copy link
Member

@jmacd please add this to the sampling project board when ready

@trask
Copy link
Member

trask commented Oct 15, 2024

@HaloFour is this issue the same as (the Java implementation) open-telemetry/opentelemetry-java#6057?

@HaloFour
Copy link
Author

@trask Yes, they are related. I believe I opened it to clarify whether the spec did permit the SDK to offer that opt-in for processors to export non-sampled spans.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig-issue A specific SIG should look into this before discussing at the spec spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests

6 participants