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

Ability to access parent span metadata during sampling. #886

Open
kevincox opened this issue Sep 27, 2022 · 5 comments
Open

Ability to access parent span metadata during sampling. #886

kevincox opened this issue Sep 27, 2022 · 5 comments
Labels
M-sdk release:required-for-stable Must be resolved before GA release, or nice to have before GA. S-blocked-spec Status: Blocked on open or unresolved spec triage:todo Needs to be traiged.

Comments

@kevincox
Copy link

I am attempting to simple my traces and add sampling information to them so that they can be correctly weighted when analyzed. I have code that is mostly working:

#[derive(Clone,Debug)]
struct Sampler;

impl opentelemetry::sdk::trace::ShouldSample for Sampler {
	fn should_sample(&self,
		parent_context: Option<&opentelemetry::Context>,
		trace_id: opentelemetry::trace::TraceId,
		_name: &str,
		_span_kind: &opentelemetry::trace::SpanKind,
		attributes: &opentelemetry::trace::OrderMap<opentelemetry::Key, opentelemetry::Value>,
		_links: &[opentelemetry::trace::Link],
		_instrumentation_library: &opentelemetry::sdk::InstrumentationLibrary,
	) -> opentelemetry::trace::SamplingResult {
		let sample_rate: u128 = attributes.iter()
			.find(|(k, _)| **k == opentelemetry::Key::from_static_str("sample_rate"))
			.map(|(_, v)| match v {
				opentelemetry::Value::I64(rate) => (*rate).try_into().unwrap_or(1),
				ref value => {
					tracing::error!(message_id="zien6Uas",
						?value,
						"Unexpected value for sample rate");
					1
				}
			})
			.unwrap_or(1)
			.max(1);

		let id = u128::from_le_bytes(trace_id.to_bytes());
		let target = u128::max_value() / sample_rate;

		let decision = if id < target {
			opentelemetry::trace::SamplingDecision::RecordAndSample
		} else {
			opentelemetry::trace::SamplingDecision::Drop
		};

		opentelemetry::trace::SamplingResult {
			decision,
			attributes: vec![
				opentelemetry::KeyValue::new("sampleRate", sample_rate),
			],
			trace_state: parent_context
				.map(|ctx| opentelemetry::trace::TraceContextExt::span(ctx)
					.span_context()
					.trace_state()
					.clone())
				.unwrap_or_default(),
		}
	}
}

However this only works correctly for top-level spans or spans that are children of unsampled spans. Notably the child of a span that is sampled 1/2 of the time should also have a sampleRate: 2 attribute applied so that it can be weighted correctly in analysis. However as far as I can tell there is no way to implement this using the current sampling infrastructure as you can't access any information from the parent span other than its trace ID.

@jtescher
Copy link
Member

@kevincox unfortunatley I don't believe that this is possible with the current opentelemetry sampling spec, the only data you are given access to is the parent context, you would have to have that information passed some other way if you wanted it present when sampling.

@kevincox
Copy link
Author

But this library has both spans so it would be nice if there was an optimistic way to share data between them. I guess I could set up a global cache to store this information but it would be a lot more difficult to manage.

Is there a way to add information to the context when processing a span so that it can be fetched from the parent context when processing the child span?

@jtescher
Copy link
Member

Is there a way to add information to the context when processing a span so that it can be fetched from the parent context when processing the child span?

Yeah you can use Context::with_value to add arbitrary data to the parent context. If you control the parent context you can set it explicitly when starting child spans (e.g. Tracer::start_with_context`)

@kevincox
Copy link
Author

I don't think that works quite enough, because I want to write to the "current" context but I don't see any way to do that with the current API. (Or can I pull the current context by using tracing_opentelemetry::OpenTelemetrySpanExt::context(&tracing::Span::current()) inside the sampler?) Reading from the parent context seems easy enough, the problem is just saving the sample rate of the current span into the context to be accessed as the parent context later.

@hdost hdost added S-blocked-spec Status: Blocked on open or unresolved spec release:required-for-stable Must be resolved before GA release, or nice to have before GA. S-blocked-cla Status: Issues blocked on CLA M-sdk and removed S-blocked-cla Status: Issues blocked on CLA labels Feb 21, 2024
@hdost hdost added this to the Tracing SDK Stable milestone Feb 21, 2024
@hdost
Copy link
Contributor

hdost commented Feb 21, 2024

I'm not sure if this will make it in , but we need to make a decision on this before being stable.

@hdost hdost added the triage:todo Needs to be traiged. label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-sdk release:required-for-stable Must be resolved before GA release, or nice to have before GA. S-blocked-spec Status: Blocked on open or unresolved spec triage:todo Needs to be traiged.
Projects
None yet
Development

No branches or pull requests

3 participants