search

open-telemetry / oteps

OpenTelemetry Enhancement Proposals
https://opentelemetry.io
Apache License 2.0
337 stars 164 forks source link

Probability sampler composition rules #175

Closed jmacd closed 2 years ago

jmacd commented 3 years ago

OTEP #168 spawned a very long comment thread here: https://github.com/open-telemetry/oteps/pull/168#discussion_r704955698

This PR is an update to the text of OTEP 170 with more detail on composing probability and non-probability samplers.

This changes the name used in OTEP 170 for "head trace sampling" to "parent trace sampling" to better fit the model where spans can make independent decisions.

This uses "head trace sampling" only to describe sampling decisions being made at the root of the trace.

This changes the field returned by the Sampler to be named log_adjusted_count since it applies to both the new span and the new child context, not the "head".

cc: @oertl

jmacd commented 3 years ago

@open-telemetry/specs-trace-approvers This PR updates the text of https://github.com/open-telemetry/oteps/blob/main/text/trace/0170-sampling-probability.md with detail on composing samplers, see the linked comment in this PR's description.

iNikem commented 3 years ago

There is a spec issue https://github.com/open-telemetry/opentelemetry-specification/issues/1844 titled "Composite Sampler". This Otep seems like talking about the same problem, but after quick skimming of this PR I don't see how it address that issue. Are they totally separate? Is this otep only about probabilistic samplers? E.g it does not define any rules for composing two non-probability samplers.

jmacd commented 3 years ago

@open-telemetry/specs-trace-approvers These are minor modifications to the text of OTEP 168 and 170, they do not substantially change the content they just spell it out more clearly and change a few terms.

The reason you should review this PR is that it includes a substantial tract of text to be inserted into the specification after it merges, related to the span data model discussed in https://github.com/open-telemetry/opentelemetry-specification/issues/1929.

jmacd commented 3 years ago

@open-telemetry/specs-trace-approvers please approve minor edits to the already-merged sampling OTEPs 168 and 170.

jmacd commented 2 years ago

@open-telemetry/specs-trace-approvers I would like this to merge, has several approvals already.