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

SDK: Samplers should be able to modify tracestate #856

Closed
lmolkova opened this issue Aug 21, 2020 · 7 comments · Fixed by #988
Closed

SDK: Samplers should be able to modify tracestate #856

lmolkova opened this issue Aug 21, 2020 · 7 comments · Fixed by #988
Labels
area:sampling Related to trace sampling priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory

Comments

@lmolkova
Copy link
Contributor

lmolkova commented Aug 21, 2020

What are you trying to achieve?

Samplers may need to modify tracestate to enable vendor-specific sampling algorithms.

Additional context.

One of the examples is described here: open-telemetry/oteps#135, where sampling score is calculated by the first service and downstream services can still use the score instead of re-calculating it using a potentially different algorithm.

The spec does not define type for tracestate and implementations use simple string or have a specific type.

Proposal:

  1. Update SamplingResult to have Tracestate with the type used by the OTel API specific to the language.

  2. Add requirement for SDK to populate tracestate received from sampler on span-to-be-created.

  3. [questionable] If aggregated sampler (with nested ones) is configured, outer sampler should take care of merging tracestate. Alternative is adding tracestate argument to ShouldSample callback to propagate outer changes to inner samplers.

@lmolkova lmolkova added the spec:trace Related to the specification/trace directory label Aug 21, 2020
@cijothomas cijothomas added the area:sampling Related to trace sampling label Aug 21, 2020
@anuraaga
Copy link
Contributor

Hey - just wanted to check, is this the same as in #854?

Also, do they solve open-telemetry/opentelemetry-proto#166? That is looking at adding a dedicated field for sampling decisions but if we need to propagate it anyways, just parsing it out of tracestate seems simpler.

@bogdandrutu
Copy link
Member

From @narayaruna

Use case:
In order to implement one or more custom sampling strategies. For example - control service participation in a trace. Imagine the following topology.

A -> B -> C -> D

Service A doesn't sample the incoming request however the trace context is initialized and sent downstream with sampling decision set to false.
Service B is configured to start a trace for all (100% sampling) the requests matching the path /foo. It encodes this sampling decision in the trace state so any downstream services can participate.
Service C doesn't do anything but propagates the context down.
Service D is configured to participate in the trace initiated by Service B. It looks up the trace state and makes a sampling decision to record the spans.
Basically the ask is to provide hooks/API changes to support such a use case. Specifically, in this example service B will need access to modify trace state where the sampling decision is taken. Service D will need access to the trace state to make a sampling decision.

@bogdandrutu
Copy link
Member

@anuraaga this is not resolving open-telemetry/opentelemetry-proto#166, this issue asks for the ability to modify trace-state in the Sampler API, one use-case may be to record a sampling decision and propagate it, but there are other use-cases as well. Also most likely because this is a custom field in the tracestate (vendor specific) we cannot consume it in the collector.

@anuraaga
Copy link
Contributor

@bogdandrutu Thanks, I don't quite understand the point about not being able to consume in the collector. Don't we copy the trace state in as is in OTLP which would be visible in the collector?

https://github.com/open-telemetry/opentelemetry-proto/blob/master/opentelemetry/proto/trace/v1/trace.proto#L80

If so, my understanding is the ability to mutate trace state with sampling decisions would automatically also allow exporting it to collector too, without having to add a new field.

@Oberon00
Copy link
Member

I would also expect the collector to be able to consume all custom fields. It might be equipped with a vendor-specific exporter after all that needs to read vendor-specific fields.

@andrewhsu andrewhsu added priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Aug 25, 2020
@bogdandrutu
Copy link
Member

The collector does not know how to parse vendor specific values in the TraceState so will not be able to consume that. If we do have a OTel entry in the TraceState that may be something the collector may know how to interpret.

@Oberon00
Copy link
Member

Vendor-specific modules can be added to the Collector, can't they?

@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 28, 2020
lmolkova pushed a commit to lmolkova/opentelemetry-specification that referenced this issue Sep 22, 2020
GA Spec Burndown automation moved this from Required for GA, needs action. Add label:release:required-for-ga to issues and PRs when moving them to this column. to Required for GA, done Sep 25, 2020
tigrannajaryan pushed a commit that referenced this issue Sep 25, 2020
Fixes #856

## Changes
Added `Tracestate` to `SamplingResult`

Related [oteps](https://github.com/open-telemetry/oteps) open-telemetry/oteps#135
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sampling Related to trace sampling priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
No open projects
GA Spec Burndown
  
Required/Allowed for GA, resolved.
Development

Successfully merging a pull request may close this issue.

6 participants