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

Add sampled flag to Span type #166

Open
kbrockhoff opened this issue Jul 10, 2020 · 7 comments
Open

Add sampled flag to Span type #166

kbrockhoff opened this issue Jul 10, 2020 · 7 comments
Assignees
Labels
release:after-ga Not required before GA release, and not going to work on before GA spec:traces

Comments

@kbrockhoff
Copy link
Member

Lack of a sampled flag in the Span type results in ambiguity. It implies that non-sampled traces are dropped at the instrumentation level and not forwarded through the system. If using tail-sampling decisions, this means all traces need to be marked sampled up front. In addition, it leaves no flexibility for different routing of traces. For example, routing all traces to a logging exporter and only sampled ones to a full-scale backend.

In addition, other protocols such as Zipkin have a sampled flag. The lack of this data makes it impossible to preserve full fidelity when converting between protocols.

@Oberon00
Copy link
Member

Isn't an unsampled but exported span an oxymoron? How could this happen in practice?

@kbrockhoff
Copy link
Member Author

There is work going on to do tail-based sampling in the collector. For this to work, all spans need to be exported to the collector.

@bogdandrutu
Copy link
Member

I am +1 on this

@bogdandrutu bogdandrutu added release:required-for-ga Must be resolved before GA release, or nice to have before GA priority:p3 labels Aug 11, 2020
@anuraaga
Copy link

anuraaga commented Aug 12, 2020

@kbrockhoff Do you mind describing more concretely the use case you have in mind? I think with pure tail sampling, we would expect the SDK to do 100% sampling and that wouldn't need to be exported. Is the goal to support multiple sampling decisions, e.g., not sampled for trace, but sampled for logs or for secondary sampling of a subset of the trace? I guess instead of a sampling flag, we'd want a list of SamplingDecision, each can be namespaced to a backend or system by a user.

Also I think this would need to be propagated too for certain types of secondary sampling?

@andrewhsu andrewhsu added this to Required for GA, needs action. Add label:release:required-for-ga to issues and PRs when moving them to this column. in GA Spec Burndown Aug 13, 2020
@kbrockhoff
Copy link
Member Author

There are several use cases for having a sampled flag.

  1. Using tail-based sampling where the team controlling the tail-based sampling and monitoring does not have control over upfront sampling configuration. This can occur in a supply chain with multiple organizations or even a siloed single organization. In this use case having a list of SamplingDecision as suggested by @anuraaga is an even better idea.

  2. Using protobuf as a general storage mechanism for tracing data. An example would be compact storage of a test data set.

In either case, adequate information can potentially be stored in the TraceState field but would then need to be parsed. I can be talked into either this approach or the SamplingDecision approach.

@andrewhsu
Copy link
Member

@arminru from the bulk move of P3 required-for-ga issues to after-ga earlier today during the issue triage mtg with TC, i noticed this issue was skipped. should the labels for tracking this issue be updated to something else?

@arminru
Copy link
Member

arminru commented Sep 29, 2020

This can be added after-ga, if still desired, yes.
@bogdandrutu you were +1 on ths, WDYT?

@andrewhsu andrewhsu added release:after-ga Not required before GA release, and not going to work on before GA and removed release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Oct 9, 2020
@andrewhsu andrewhsu removed this from Required for GA, needs action. Add label:release:required-for-ga to issues and PRs when moving them to this column. in GA Spec Burndown Oct 16, 2020
@andrewhsu andrewhsu added release:allowed-for-ga Editorial changes that can still be added to the GA spec since they don't require action by SIGs and removed release:after-ga Not required before GA release, and not going to work on before GA labels Oct 23, 2020
@andrewhsu andrewhsu added this to Required/Allowed for GA, todo. in GA Spec Burndown Nov 12, 2020
@bogdandrutu bogdandrutu added release:after-ga Not required before GA release, and not going to work on before GA and removed priority:p3 release:allowed-for-ga Editorial changes that can still be added to the GA spec since they don't require action by SIGs labels Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:after-ga Not required before GA release, and not going to work on before GA spec:traces
Projects
No open projects
GA Spec Burndown
  
Required/Allowed for GA, todo.
Development

No branches or pull requests

7 participants