From 207ba2ff56b837681dd60b0cba347ed1a3a34510 Mon Sep 17 00:00:00 2001 From: Kanwaldeep Dang Date: Mon, 18 May 2020 10:45:34 -0700 Subject: [PATCH 01/23] Proposal for sampling.priority --- text/trace/0000-sampling-priority.md | 83 ++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 text/trace/0000-sampling-priority.md diff --git a/text/trace/0000-sampling-priority.md b/text/trace/0000-sampling-priority.md new file mode 100644 index 000000000..0477e854d --- /dev/null +++ b/text/trace/0000-sampling-priority.md @@ -0,0 +1,83 @@ +# Associating sampling priority with the trace using tracestate + +Propagating `sampling.priority` (calculated by probability sampler) in a +`tracestate` will enable a better experience for system with the independent +stateless configuration of components sampling. + +## Motivation + +Consistent sampling decision made in each app of a distributed trace is +important for better user experience of traces analysis. Consistency is achieved +by aligning stateless hashing algorithm used to make a decision based on +trace-id or explicitly propagating sampling flag. + +Increasingly more apps participate in distributed tracing. With the +standardized wire format for context propagation, there is bigger chance that +approach for sampling chosen by one app may not match another app sampling +policy. Since coordination of sampling policies across many apps not always +possible, OpenTelemetry must provide a way to share sampling hints between apps. + +Beyond sampling hints infrastructure, it is beneficial for the community to +agree on standard behavior and hints exposed by probability sampler. + +The `sampling.priority` property is used by many vendors for various purposes. +Propagating this field alongside the trace will allow for many improvements and +as a very minimum will simplify transition of customers from SDKs using +different `trace-id` based hash functions to OpenTelemetry SDK. + +Also see discussion here: https://github.com/open-telemetry/opentelemetry-specification/pull/570 + +## Explanation + +### Sampling hints exchange + +OpenTelemetry SDK has an infra to expose sampling hints as span attributes today +via +[SamplingResult](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#shouldsample) +class. + +This recommendation suggests OpenTelemetry will allow to return an updated +`tracestate` that will be used for a newly created `Span`. This will allow +samplers from a single vendor coordinate sampling decisions across the +different components of the trace. + +### Probability Sampler default behavior + +The default behavior of probability sampler is to calculate a stateless hash +function of `trace-id` to make a sampling decision. + +It is suggested to extend the sampler behavior to expose a `tracestate` field +called `sampling.priority` with the integer value `[0-1000]` that will indicate +sampling priority of the current span. + +This will allow to align sampling algorithms between various components. +Especially for the transition scenarios from the different SDKs. + +Beyond the transition scenarios, propagation of `sampling.priority` will allow +to build more sophisticated logic of probability calculation that may be +customized to be consistent across properties beyond `trace-id`. Like +session-aware samplers. It also allows to use more secure probability +calculation that is NOT based on incoming parameters like `trace-id`. Especially +if there is an option to configure sampler to NOT respect `sampling.priority` +from the incoming `tracestate`. + +## Internal details + +Options on `sampling.priority` default behavior. Exposing `sampling.priority` is +not always needed. It also may be desired to not respect incoming +`sampling.priority`. So `ProbabilitySampler` may be configured out of the box to +respect incoming `sampling.priority`. And inserting it into `tracestate` when +not present. A setting can be exposed to NOT write `sampling.priority` to +`tracestate`. + + +## Trade-offs and mitigations + + +## Prior art and alternatives + + +## Open questions + + +## Future possibilities From 0510dfd092b2a1ae3ad4b17b9831ae833165e600 Mon Sep 17 00:00:00 2001 From: Kanwaldeep Dang Date: Mon, 18 May 2020 12:22:16 -0700 Subject: [PATCH 02/23] Renamed the file and updated the contents for the markdown checks --- ...{0000-sampling-priority.md => 0107-sampling-priority.md} | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) rename text/trace/{0000-sampling-priority.md => 0107-sampling-priority.md} (97%) diff --git a/text/trace/0000-sampling-priority.md b/text/trace/0107-sampling-priority.md similarity index 97% rename from text/trace/0000-sampling-priority.md rename to text/trace/0107-sampling-priority.md index 0477e854d..b8ad27f6e 100644 --- a/text/trace/0000-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -25,7 +25,7 @@ Propagating this field alongside the trace will allow for many improvements and as a very minimum will simplify transition of customers from SDKs using different `trace-id` based hash functions to OpenTelemetry SDK. -Also see discussion here: https://github.com/open-telemetry/opentelemetry-specification/pull/570 +Also see discussion here: ## Explanation @@ -70,14 +70,10 @@ respect incoming `sampling.priority`. And inserting it into `tracestate` when not present. A setting can be exposed to NOT write `sampling.priority` to `tracestate`. - ## Trade-offs and mitigations - ## Prior art and alternatives - ## Open questions - ## Future possibilities From 096d10bb2ff40da16c17a6bf43228e6410a47697 Mon Sep 17 00:00:00 2001 From: Kanwaldeep Dang Date: Thu, 28 May 2020 00:11:18 -0700 Subject: [PATCH 03/23] Added more details based on feedback --- text/trace/0107-sampling-priority.md | 36 +++++++++++++++------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index b8ad27f6e..f3dc4a6f2 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -7,15 +7,16 @@ stateless configuration of components sampling. ## Motivation Consistent sampling decision made in each app of a distributed trace is -important for better user experience of traces analysis. Consistency is achieved -by aligning stateless hashing algorithm used to make a decision based on -trace-id or explicitly propagating sampling flag. +important for better user experience of trace analysis. Consistency is achieved +by following means: +- Same hashing algorithms used across all apps in a trace. +- Sampling flag propagated from the head component/app is used by downstream apps to sample in a given trace. -Increasingly more apps participate in distributed tracing. With the -standardized wire format for context propagation, there is bigger chance that -approach for sampling chosen by one app may not match another app sampling -policy. Since coordination of sampling policies across many apps not always -possible, OpenTelemetry must provide a way to share sampling hints between apps. +Increasingly more apps participate in distributed tracing. With the +standardized wire format for context propagation, there is bigger chance that +algorithm for sampling chosen by one app may not match another app. +Since coordination of sampling algorithms across many apps not always possible, +OpenTelemetry must provide a way to share sampling hints between apps. Beyond sampling hints infrastructure, it is beneficial for the community to agree on standard behavior and hints exposed by probability sampler. @@ -25,13 +26,13 @@ Propagating this field alongside the trace will allow for many improvements and as a very minimum will simplify transition of customers from SDKs using different `trace-id` based hash functions to OpenTelemetry SDK. -Also see discussion here: +Also see related discussions on probability sampler here: ## Explanation ### Sampling hints exchange -OpenTelemetry SDK has an infra to expose sampling hints as span attributes today +OpenTelemetry SDK has an infrastructure to expose sampling hints as span attributes today via [SamplingResult](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#shouldsample) class. @@ -48,18 +49,18 @@ function of `trace-id` to make a sampling decision. It is suggested to extend the sampler behavior to expose a `tracestate` field called `sampling.priority` with the integer value `[0-1000]` that will indicate -sampling priority of the current span. +sampling priority of the current span. The sampling.priority is calculated by rounding +the output of the hash function by factor of 1000. For example calculated probability +of 0.461 the sampling.priority would be 461. This will allow to align sampling algorithms between various components. -Especially for the transition scenarios from the different SDKs. +Especially for the transition scenarios where components are using different +versions of the SDKs with different sampling algorithms. The components would +respect the sampling.priority if passed in to make sampling decision. Beyond the transition scenarios, propagation of `sampling.priority` will allow to build more sophisticated logic of probability calculation that may be -customized to be consistent across properties beyond `trace-id`. Like -session-aware samplers. It also allows to use more secure probability -calculation that is NOT based on incoming parameters like `trace-id`. Especially -if there is an option to configure sampler to NOT respect `sampling.priority` -from the incoming `tracestate`. +customized to be consistent across properties beyond `trace-id`. ## Internal details @@ -73,6 +74,7 @@ not present. A setting can be exposed to NOT write `sampling.priority` to ## Trade-offs and mitigations ## Prior art and alternatives +https://github.com/open-telemetry/opentelemetry-collector/blob/60b03d0d2d503351501291b30836d2126487a741/processor/samplingprocessor/probabilisticsamplerprocessor/testdata/config.yaml#L10 ## Open questions From c41aa8a1480c4ee0f6d23abccd9abb279a6078ed Mon Sep 17 00:00:00 2001 From: Kanwaldeep Dang Date: Thu, 28 May 2020 00:15:48 -0700 Subject: [PATCH 04/23] Fixed link --- text/trace/0107-sampling-priority.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index f3dc4a6f2..7c159ada0 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -74,7 +74,7 @@ not present. A setting can be exposed to NOT write `sampling.priority` to ## Trade-offs and mitigations ## Prior art and alternatives -https://github.com/open-telemetry/opentelemetry-collector/blob/60b03d0d2d503351501291b30836d2126487a741/processor/samplingprocessor/probabilisticsamplerprocessor/testdata/config.yaml#L10 + ## Open questions From eed164f472b658928516b08b1a6dc5fb943dfc13 Mon Sep 17 00:00:00 2001 From: Kanwaldeep Dang Date: Sun, 7 Jun 2020 15:18:56 -0700 Subject: [PATCH 05/23] Updated based on feedback. Added example --- text/trace/0107-sampling-priority.md | 42 +++++++++++++++------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index 7c159ada0..ce52773c5 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -9,13 +9,14 @@ stateless configuration of components sampling. Consistent sampling decision made in each app of a distributed trace is important for better user experience of trace analysis. Consistency is achieved by following means: + - Same hashing algorithms used across all apps in a trace. - Sampling flag propagated from the head component/app is used by downstream apps to sample in a given trace. -Increasingly more apps participate in distributed tracing. With the -standardized wire format for context propagation, there is bigger chance that -algorithm for sampling chosen by one app may not match another app. -Since coordination of sampling algorithms across many apps not always possible, +Increasingly more apps participate in distributed tracing. With the +standardized wire format for context propagation, there is bigger chance that +algorithm for sampling chosen by one app may not match another app. +Since coordination of sampling algorithms across many apps not always possible, OpenTelemetry must provide a way to share sampling hints between apps. Beyond sampling hints infrastructure, it is beneficial for the community to @@ -37,10 +38,10 @@ via [SamplingResult](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#shouldsample) class. -This recommendation suggests OpenTelemetry will allow to return an updated -`tracestate` that will be used for a newly created `Span`. This will allow -samplers from a single vendor coordinate sampling decisions across the -different components of the trace. +This recommendation suggests OpenTelemetry will allow to return the +`sampling.priority` that will be appended to the tracestate for a newly created `Span`. +This will allow samplers from a single vendor coordinate sampling decision across +the different components of the trace. ### Probability Sampler default behavior @@ -48,19 +49,21 @@ The default behavior of probability sampler is to calculate a stateless hash function of `trace-id` to make a sampling decision. It is suggested to extend the sampler behavior to expose a `tracestate` field -called `sampling.priority` with the integer value `[0-1000]` that will indicate -sampling priority of the current span. The sampling.priority is calculated by rounding -the output of the hash function by factor of 1000. For example calculated probability -of 0.461 the sampling.priority would be 461. +called `sampling.priority` with the floating point value `[0-1]` that will indicate +sampling priority of the current span. The sampling.priority is the probability as calculated +by the hashing function. This will allow to align sampling algorithms between various components. -Especially for the transition scenarios where components are using different -versions of the SDKs with different sampling algorithms. The components would -respect the sampling.priority if passed in to make sampling decision. - -Beyond the transition scenarios, propagation of `sampling.priority` will allow -to build more sophisticated logic of probability calculation that may be -customized to be consistent across properties beyond `trace-id`. +Especially for the transition scenarios where components are using different +versions of the SDKs with different sampling algorithms. The components would +respect the sampling.priority if passed in to make sampling decision and not re-calculate +the sampling probability. So let's assume head component A started the trace and +sampled in 60% of the traces and then next component B has sampling policy of +30% but is using different sampling algorithm. In this scenario how can we make sure +that the 30% of the traces sampled in by component B are subset of the 60% traces +sampled in by component A. So, by passing the sampling priority component B can use +the passed in value to make the decision instead of re-calculating with possibly a +different probability algorithm. ## Internal details @@ -74,6 +77,7 @@ not present. A setting can be exposed to NOT write `sampling.priority` to ## Trade-offs and mitigations ## Prior art and alternatives + ## Open questions From 146d9635ab1a6274a61bdb115fc47756bf2b9997 Mon Sep 17 00:00:00 2001 From: Kanwaldeep Dang Date: Sun, 7 Jun 2020 15:32:51 -0700 Subject: [PATCH 06/23] Minor styling fixes --- text/trace/0107-sampling-priority.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index ce52773c5..6769f53ca 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -39,8 +39,8 @@ via class. This recommendation suggests OpenTelemetry will allow to return the -`sampling.priority` that will be appended to the tracestate for a newly created `Span`. -This will allow samplers from a single vendor coordinate sampling decision across +`sampling.priority` that will be appended to the tracestate for a newly created `Span`. +This will allow samplers from a single vendor coordinate sampling decision across the different components of the trace. ### Probability Sampler default behavior @@ -58,7 +58,7 @@ Especially for the transition scenarios where components are using different versions of the SDKs with different sampling algorithms. The components would respect the sampling.priority if passed in to make sampling decision and not re-calculate the sampling probability. So let's assume head component A started the trace and -sampled in 60% of the traces and then next component B has sampling policy of +sampled in 60% of the traces and then next component B has sampling policy of 30% but is using different sampling algorithm. In this scenario how can we make sure that the 30% of the traces sampled in by component B are subset of the 60% traces sampled in by component A. So, by passing the sampling priority component B can use From 0295785a3250afebcf955b1f7687848c2f0c7446 Mon Sep 17 00:00:00 2001 From: Kanwaldeep Dang Date: Sun, 7 Jun 2020 15:48:46 -0700 Subject: [PATCH 07/23] Fixing lint error --- text/trace/0107-sampling-priority.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index 6769f53ca..03520e975 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -51,7 +51,7 @@ function of `trace-id` to make a sampling decision. It is suggested to extend the sampler behavior to expose a `tracestate` field called `sampling.priority` with the floating point value `[0-1]` that will indicate sampling priority of the current span. The sampling.priority is the probability as calculated -by the hashing function. +by the hashing function. This will allow to align sampling algorithms between various components. Especially for the transition scenarios where components are using different From 9b0dcdcd26454e848ff16d170b2920d947aea305 Mon Sep 17 00:00:00 2001 From: Liudmila Date: Fri, 14 Aug 2020 14:32:41 -0700 Subject: [PATCH 08/23] Add details and explanations --- text/trace/0107-sampling-priority.md | 197 +++++++++++++++++++-------- 1 file changed, 139 insertions(+), 58 deletions(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index 03520e975..0c296e664 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -1,85 +1,166 @@ -# Associating sampling priority with the trace using tracestate +# Associating sampling priority with the trace -Propagating `sampling.priority` (calculated by probability sampler) in a -`tracestate` will enable a better experience for system with the independent -stateless configuration of components sampling. +Enable consistent sampling across distributed application with different +sampling rates and probability calculation algorithms. + +## TL;DR + +**Priority** in the scope of this spec is a score associated with trace. +It's calculated when trace starts and flows in the `tracestate`, it's +used by samplers to make consistent sampling decisions. + +Service that starts the trace calculates sampling priority and adds it to the +tracestate so downstream services can use it to make their sampling decisions +without re-calculating priority as a function of trace-id (or trace-flags). ## Motivation +The goal is to enable a mechanism for consistent (best effort) sampling +between services with different sampling rates and different probability +calculation algorithms (for interoperability with existing tracing tools). + Consistent sampling decision made in each app of a distributed trace is important for better user experience of trace analysis. Consistency is achieved by following means: -- Same hashing algorithms used across all apps in a trace. -- Sampling flag propagated from the head component/app is used by downstream apps to sample in a given trace. +1. Same hashing algorithms used across all apps in a trace. + Coordination of sampling algorithms across multiple apps not always possible: + for example existing components in a system use vendor-specific + tracing tool (pre-OpenTelemetry and update is hard to justify) while there + is a desire to use OpenTelemtery for new components. -Increasingly more apps participate in distributed tracing. With the -standardized wire format for context propagation, there is bigger chance that -algorithm for sampling chosen by one app may not match another app. -Since coordination of sampling algorithms across many apps not always possible, -OpenTelemetry must provide a way to share sampling hints between apps. +2. Sampling flag propagated from the head component/app is used by downstream + apps to sample in a given trace. + It requires to trust upstream decision and does not allow to have different + sampling rates across different components. -Beyond sampling hints infrastructure, it is beneficial for the community to -agree on standard behavior and hints exposed by probability sampler. +## Explanation -The `sampling.priority` property is used by many vendors for various purposes. -Propagating this field alongside the trace will allow for many improvements and -as a very minimum will simplify transition of customers from SDKs using -different `trace-id` based hash functions to OpenTelemetry SDK. +Sampling propabaility is generated by the first service to make sampling +decision. It's a random float number in [0, 1] range. +Priority is stamped on the span and also propagated further within `tracestate`. + +Next service reads priority from `tracestate` (instead of calculating it from +trace-id) and compares it with its sampling rate to make sampling decision. + +Priority is also exposed through span attributes. Vendors can leverage it +to sort traces based on their completeness: the lower the value of priority is, +the higher the chance it was sampled it by each component. + +### Example + +``` ++----------------------+ +----------------------+ +----------------------+ ++ Service-A (rate 0.6) + --> + Service-B (rate 0.1) + --> + Service-C (rate 0.5) + ++-------------- -------+ +----------------------+ +----------------------+ +``` + +1. Service-A receives a request + - starts a new trace, generates random trace-id + - generates priority: `0.17935003`. It's **smaller** than sampling rate + (`0.6`), so decision is `RECORD_AND_SAMPLED` + - span gets a new attribute `sampling.priority = 0.17935003` + - tracestate is modified `sampling.priority=0.17935003` +2. Service-B gets request from A + - reads trace-context from headers and `sampling.priority` from the + tracestate + - decision is `NOT_RECORD` as `0.17935003` is **bigger** than its + sampling rate (0.1) +3. Service-C get a request from B + - reads trace-context from headers and `sampling.priority` from the + tracestate + - decision is `RECORD_AND_SAMPLED` as `0.17935003` is **smaller** than its + sampling rate (0.5) + - span gets a new attribute `sampling.priority = 0.17935003` + - tracestate is left untouched + +As a result, spans from Service-A and Service-C are exported. +It's not possible to restore relationship between A and C without B and the +trace is broken, but Service-C can trace their own requests regardless of B's +sampling rate and B can have smaller tracing budget regardless of A's decisions. +All of them can still debug integration issues using common trace-id. + +Vendors can pick the most complete traces sorting them by priority. -Also see related discussions on probability sampler here: +## Internal details -## Explanation +- Service that starts a trace makes sampling decision. It's configured to use +`ExternalPrioritySampler`(name TBD) is configured by user. Within `ShouldSample` +callback sampler + - generates random float priority (TBD precision) in [0, 1] interval + - makes sampling decision by comparing generated priority to configured rate + - if decision is `RECORD` (or `RECORD_AND_SAMPLED`), sampler adds + `sampling.priority` attribute to attributes collection of to-be-created span + - regardless of sampling decision: prepends `sampling.priority` key-value pair + into tracestate of to-be-created span +- Downstream service continues a trace but has different sampling rate (it's + also configured to use `ExternalPrioritySampler`) + - `ExternalPrioritySampler.ShouldSample` checks if priority is provided in + `tracestate`. + - makes sampling decision by comparing upstream-generated priority with its + sampling rate + - if span will be recorded: sampler adds `sampling.priority` attribute to + attributes collection of to-be-created span + +### Specification Delta + +1. Add `SamplingResult.Tracestate` field: sampler should be able to assign a + new tracestate for to-be-created span +2. Add convention for `sampling.priority` attribute on span +3. Add `ExternalPrioritySampler` implementation of `Sampler` + +### Trade-offs and mitigations + +This change would be the first (AFAIK) common use case of the `tracestate`. +It comes with bandwidth as well as performance overhead: tracestate could have +been just propagated [blindly](https://github.com/open-telemetry/opentelemetry-specification/issues/478). +and this overhead is made even before sampling decision and cannot be mitigated. + +Customers should configure it explicitly to avoid the overhead. Vendors may +gradually update their existing solutions to support external priority and +interoperate with OpenTelemetry should recommend customers to configure such +sampler. -### Sampling hints exchange +## Prior art and alternatives -OpenTelemetry SDK has an infrastructure to expose sampling hints as span attributes today -via -[SamplingResult](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#shouldsample) -class. +[OpenTelemetry collector](https://github.com/open-telemetry/opentelemetry-collector/blob/60b03d0d2d503351501291b30836d2126487a741/processor/samplingprocessor/probabilisticsamplerprocessor/testdata/config.yaml#L10) uses `sampling.priority` +[OpenTracing](https://github.com/opentracing/specification/blob/master/semantic_conventions.md) +defined implementation-specific hint for sampler to prioritize recording a span. +Related discussions on [Probability sampler](https://github.com/open-telemetry/opentelemetry-specification/pull/570) -This recommendation suggests OpenTelemetry will allow to return the -`sampling.priority` that will be appended to the tracestate for a newly created `Span`. -This will allow samplers from a single vendor coordinate sampling decision across -the different components of the trace. +## Open questions -### Probability Sampler default behavior +### Priority calculation -The default behavior of probability sampler is to calculate a stateless hash -function of `trace-id` to make a sampling decision. +This spec suggests to generate priority randomly to achieve uniform +distribution. -It is suggested to extend the sampler behavior to expose a `tracestate` field -called `sampling.priority` with the floating point value `[0-1]` that will indicate -sampling priority of the current span. The sampling.priority is the probability as calculated -by the hashing function. +Assuming trace-ids are uniformly distributed, `ProbabilitySampler` can generate +priority, so the flow could look like this: -This will allow to align sampling algorithms between various components. -Especially for the transition scenarios where components are using different -versions of the SDKs with different sampling algorithms. The components would -respect the sampling.priority if passed in to make sampling decision and not re-calculate -the sampling probability. So let's assume head component A started the trace and -sampled in 60% of the traces and then next component B has sampling policy of -30% but is using different sampling algorithm. In this scenario how can we make sure -that the 30% of the traces sampled in by component B are subset of the 60% traces -sampled in by component A. So, by passing the sampling priority component B can use -the passed in value to make the decision instead of re-calculating with possibly a -different probability algorithm. +`ExternalprioritySampler.ShouldSample`: -## Internal details +- checks if `sampling.priority` is available in the tracestate +- if it's not there, invokes `ProbabilitySampler`, which calculates priority + and populates it on the attributes +- updates tracestate -Options on `sampling.priority` default behavior. Exposing `sampling.priority` is -not always needed. It also may be desired to not respect incoming -`sampling.priority`. So `ProbabilitySampler` may be configured out of the box to -respect incoming `sampling.priority`. And inserting it into `tracestate` when -not present. A setting can be exposed to NOT write `sampling.priority` to -`tracestate`. +**Pros** +Fallback to `ProbabilitySampler` improves the case when `tracestate` is trimmed +so that is a chace for consistent sampling. -## Trade-offs and mitigations +**Cons** +There is no requirement for trace-ids to be uniformly distributed +No clear boundary betwwen `ProbabilitySampler` and `ExternalprioritySampler` +needs to expose this priority, even if there is no wrapper. -## Prior art and alternatives +## Future possibilities - +### Attribute vs field on the span to-be-created -## Open questions +Collection of attributes that is passed to sampler is empty by default to +minimize perf impact. Propagating priority back from sampler to span requires +to initialize the collection. -## Future possibilities +Creating a new float field on `SamplingDecision` could be an alternative. +It'd require also adding similar property on Span/SpanData. From 2509e5e61b030bcf8d3103e0ceaad3519cba6770 Mon Sep 17 00:00:00 2001 From: Liudmila Date: Fri, 14 Aug 2020 14:44:09 -0700 Subject: [PATCH 09/23] mitigation update --- text/trace/0107-sampling-priority.md | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index 0c296e664..b6d303baa 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -112,14 +112,20 @@ callback sampler ### Trade-offs and mitigations This change would be the first (AFAIK) common use case of the `tracestate`. -It comes with bandwidth as well as performance overhead: tracestate could have +It comes with bandwidth and performance overhead: `tracestate` could have been just propagated [blindly](https://github.com/open-telemetry/opentelemetry-specification/issues/478). -and this overhead is made even before sampling decision and cannot be mitigated. +and the overhead is made before sampling decision and cannot be mitigated. -Customers should configure it explicitly to avoid the overhead. Vendors may -gradually update their existing solutions to support external priority and -interoperate with OpenTelemetry should recommend customers to configure such -sampler. +Customers should configure it explicitly to avoid the overhead in the default +case when interoperability is not necessary. + +Vendors may gradually update their existing solutions to support external +priority in order to interoperate with OpenTelemetry and should recommend +customers to configure such sampler. + +It may be the case that after migration to OpenTelemtery is finalized, the need +of `sampling.priority` will decrease and customers can remove +`ExternalPrioritySampler` from configuration. ## Prior art and alternatives From 65b6b404320cb65a8a757486f081146acab48bab Mon Sep 17 00:00:00 2001 From: Liudmila Date: Fri, 14 Aug 2020 15:32:17 -0700 Subject: [PATCH 10/23] typos --- text/trace/0107-sampling-priority.md | 32 ++++++++++++++++------------ 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index b6d303baa..c087af9df 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -10,7 +10,7 @@ It's calculated when trace starts and flows in the `tracestate`, it's used by samplers to make consistent sampling decisions. Service that starts the trace calculates sampling priority and adds it to the -tracestate so downstream services can use it to make their sampling decisions +`tracestate` so downstream services can use it to make their sampling decisions without re-calculating priority as a function of trace-id (or trace-flags). ## Motivation @@ -27,7 +27,7 @@ by following means: Coordination of sampling algorithms across multiple apps not always possible: for example existing components in a system use vendor-specific tracing tool (pre-OpenTelemetry and update is hard to justify) while there - is a desire to use OpenTelemtery for new components. + is a desire to use OpenTelemetry for new components. 2. Sampling flag propagated from the head component/app is used by downstream apps to sample in a given trace. @@ -123,7 +123,7 @@ Vendors may gradually update their existing solutions to support external priority in order to interoperate with OpenTelemetry and should recommend customers to configure such sampler. -It may be the case that after migration to OpenTelemtery is finalized, the need +It may be the case that after migration to OpenTelemetry is finalized, the need of `sampling.priority` will decrease and customers can remove `ExternalPrioritySampler` from configuration. @@ -136,7 +136,7 @@ Related discussions on [Probability sampler](https://github.com/open-telemetry/o ## Open questions -### Priority calculation +### Priority calculation: can we use ProbabilitySampler? This spec suggests to generate priority randomly to achieve uniform distribution. @@ -144,29 +144,33 @@ distribution. Assuming trace-ids are uniformly distributed, `ProbabilitySampler` can generate priority, so the flow could look like this: -`ExternalprioritySampler.ShouldSample`: +`ExternalPrioritySampler.ShouldSample`: - checks if `sampling.priority` is available in the tracestate - if it's not there, invokes `ProbabilitySampler`, which calculates priority and populates it on the attributes - updates tracestate -**Pros** +#### Pros + Fallback to `ProbabilitySampler` improves the case when `tracestate` is trimmed -so that is a chace for consistent sampling. +so there is a chance sampling could be consistent if same probability +calculation algorithm was used. + +#### Cons -**Cons** -There is no requirement for trace-ids to be uniformly distributed -No clear boundary betwwen `ProbabilitySampler` and `ExternalprioritySampler` -needs to expose this priority, even if there is no wrapper. +- There is no requirement for trace-ids to be uniformly distributed +- No clear boundary between `ProbabilitySampler` and `ExternalPrioritySampler`. +`ProbabilitySampler` needs to set priority in attribute even if there is no +`ExternalPrioritySampler`. ## Future possibilities ### Attribute vs field on the span to-be-created -Collection of attributes that is passed to sampler is empty by default to +Collection of attributes which is passed to sampler is empty by default to minimize perf impact. Propagating priority back from sampler to span requires to initialize the collection. -Creating a new float field on `SamplingDecision` could be an alternative. -It'd require also adding similar property on Span/SpanData. +Creating a new float field on `SamplingDecision` could be an alternative. +It'd also require adding similar property on Span/SpanData. From 122eb25e26445415184e5990ce60f53cd08cbe06 Mon Sep 17 00:00:00 2001 From: Liudmila Date: Mon, 17 Aug 2020 13:04:18 -0700 Subject: [PATCH 11/23] add link to PoC --- text/trace/0107-sampling-priority.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index c087af9df..8aac9286a 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -102,6 +102,9 @@ callback sampler - if span will be recorded: sampler adds `sampling.priority` attribute to attributes collection of to-be-created span +Here is a [proof of concept](https://github.com/lmolkova/opentelemetry-dotnet/pull/1) +in .NET. + ### Specification Delta 1. Add `SamplingResult.Tracestate` field: sampler should be able to assign a From bfc6f3750375584cf5cb75c71e2554b1849f42e3 Mon Sep 17 00:00:00 2001 From: Liudmila Date: Mon, 17 Aug 2020 13:09:01 -0700 Subject: [PATCH 12/23] float precision --- text/trace/0107-sampling-priority.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index 8aac9286a..cdb747ede 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -87,7 +87,7 @@ Vendors can pick the most complete traces sorting them by priority. - Service that starts a trace makes sampling decision. It's configured to use `ExternalPrioritySampler`(name TBD) is configured by user. Within `ShouldSample` callback sampler - - generates random float priority (TBD precision) in [0, 1] interval + - generates random float priority (~7 decimal digits) in [0, 1] interval - makes sampling decision by comparing generated priority to configured rate - if decision is `RECORD` (or `RECORD_AND_SAMPLED`), sampler adds `sampling.priority` attribute to attributes collection of to-be-created span From 597a78f70942728af94b08d9badb5c74835f29bb Mon Sep 17 00:00:00 2001 From: Liudmila Date: Mon, 17 Aug 2020 13:12:27 -0700 Subject: [PATCH 13/23] float precision --- text/trace/0107-sampling-priority.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index cdb747ede..0e85dddca 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -87,7 +87,7 @@ Vendors can pick the most complete traces sorting them by priority. - Service that starts a trace makes sampling decision. It's configured to use `ExternalPrioritySampler`(name TBD) is configured by user. Within `ShouldSample` callback sampler - - generates random float priority (~7 decimal digits) in [0, 1] interval + - generates random float priority (6-9 digits) in [0, 1] interval - makes sampling decision by comparing generated priority to configured rate - if decision is `RECORD` (or `RECORD_AND_SAMPLED`), sampler adds `sampling.priority` attribute to attributes collection of to-be-created span From b17dfc69e9989f6fdaf2bd7993c732822ee7715c Mon Sep 17 00:00:00 2001 From: Liudmila Date: Tue, 18 Aug 2020 12:14:06 -0700 Subject: [PATCH 14/23] semantic convention vs extensible struct --- text/trace/0107-sampling-priority.md | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index 0e85dddca..38553fec4 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -109,7 +109,8 @@ in .NET. 1. Add `SamplingResult.Tracestate` field: sampler should be able to assign a new tracestate for to-be-created span -2. Add convention for `sampling.priority` attribute on span +2. Add convention for `sampling.priority` attribute on span (TBC). Check out + [open questions](open-questions) regarding attribute vs special field. 3. Add `ExternalPrioritySampler` implementation of `Sampler` ### Trade-offs and mitigations @@ -167,8 +168,6 @@ calculation algorithm was used. `ProbabilitySampler` needs to set priority in attribute even if there is no `ExternalPrioritySampler`. -## Future possibilities - ### Attribute vs field on the span to-be-created Collection of attributes which is passed to sampler is empty by default to @@ -177,3 +176,25 @@ to initialize the collection. Creating a new float field on `SamplingDecision` could be an alternative. It'd also require adding similar property on Span/SpanData. + +There are other scenarios when sampling information is useful for +exporter: e.g. sampling rate (or inverse value: count of spans +this span represent). Exporters can use it to estimate metrics. + +Populating all sampling information on all spans may be inefficient in terms of +event payload size and storage while being useful for a subset of vendors. + +Extensible solution may look like a `SamplingInfo` struct that carries all +fields exporters may need. + +``` +struct SamplingInfo + Priority, + Rate/Count, + ... +``` + +`SamplingResult` would allow sampler for fill it for the span-to-be-created. +`Span` and its exportable representations will also need to be updated. + +## Future possibilities From f096d0262b815375c763eafa78648e4349f48912 Mon Sep 17 00:00:00 2001 From: Liudmila Date: Thu, 20 Aug 2020 09:13:10 -0700 Subject: [PATCH 15/23] rename to score, minor fixes --- text/trace/0107-sampling-priority.md | 102 +++++++++++++++------------ 1 file changed, 56 insertions(+), 46 deletions(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index 38553fec4..2092ccfb4 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -1,17 +1,20 @@ -# Associating sampling priority with the trace +# Associating sampling score with the trace Enable consistent sampling across distributed application with different sampling rates and probability calculation algorithms. ## TL;DR -**Priority** in the scope of this spec is a score associated with trace. -It's calculated when trace starts and flows in the `tracestate`, it's -used by samplers to make consistent sampling decisions. +**Score** in the scope of this spec is a floating point number associated with +the trace. It's calculated when trace starts and flows in the `tracestate`, +it's used by samplers to make consistent sampling decisions. +*Score* is not related to sampling *rate* (aka *probability* - sampler's +configuration not specific to trace). -Service that starts the trace calculates sampling priority and adds it to the -`tracestate` so downstream services can use it to make their sampling decisions -without re-calculating priority as a function of trace-id (or trace-flags). +Service that starts the trace calculates the score and adds it to the +`tracestate` so downstream services can re-use it to make their sampling +decisions instead of re-calculating score as a function of trace-id +(or trace-flags). ## Motivation @@ -38,13 +41,13 @@ by following means: Sampling propabaility is generated by the first service to make sampling decision. It's a random float number in [0, 1] range. -Priority is stamped on the span and also propagated further within `tracestate`. +Score is stamped on the span and also propagated further within `tracestate`. -Next service reads priority from `tracestate` (instead of calculating it from +Next service reads score from `tracestate` (instead of calculating it from trace-id) and compares it with its sampling rate to make sampling decision. -Priority is also exposed through span attributes. Vendors can leverage it -to sort traces based on their completeness: the lower the value of priority is, +Score is also exposed through span attributes. Vendors can leverage it +to sort traces based on their completeness: the lower the value of score is, the higher the chance it was sampled it by each component. ### Example @@ -57,21 +60,21 @@ the higher the chance it was sampled it by each component. 1. Service-A receives a request - starts a new trace, generates random trace-id - - generates priority: `0.17935003`. It's **smaller** than sampling rate + - generates score: `0.17935003`. It's **smaller** than sampling rate (`0.6`), so decision is `RECORD_AND_SAMPLED` - - span gets a new attribute `sampling.priority = 0.17935003` - - tracestate is modified `sampling.priority=0.17935003` + - span gets a new attribute `sampling.score = 0.17935003` + - tracestate is modified `sampling.score=0.17935003` 2. Service-B gets request from A - - reads trace-context from headers and `sampling.priority` from the + - reads trace-context from headers and `sampling.score` from the tracestate - decision is `NOT_RECORD` as `0.17935003` is **bigger** than its sampling rate (0.1) 3. Service-C get a request from B - - reads trace-context from headers and `sampling.priority` from the + - reads trace-context from headers and `sampling.score` from the tracestate - decision is `RECORD_AND_SAMPLED` as `0.17935003` is **smaller** than its sampling rate (0.5) - - span gets a new attribute `sampling.priority = 0.17935003` + - span gets a new attribute `sampling.score = 0.17935003` - tracestate is left untouched As a result, spans from Service-A and Service-C are exported. @@ -80,26 +83,26 @@ trace is broken, but Service-C can trace their own requests regardless of B's sampling rate and B can have smaller tracing budget regardless of A's decisions. All of them can still debug integration issues using common trace-id. -Vendors can pick the most complete traces sorting them by priority. +Vendors can pick the most complete traces sorting them by score. ## Internal details - Service that starts a trace makes sampling decision. It's configured to use -`ExternalPrioritySampler`(name TBD) is configured by user. Within `ShouldSample` +`ExternalScoreSampler`(name TBD) is configured by user. Within `ShouldSample` callback sampler - - generates random float priority (6-9 digits) in [0, 1] interval - - makes sampling decision by comparing generated priority to configured rate + - generates random float score (6-9 digits) in [0, 1] interval + - makes sampling decision by comparing generated score to configured rate - if decision is `RECORD` (or `RECORD_AND_SAMPLED`), sampler adds - `sampling.priority` attribute to attributes collection of to-be-created span - - regardless of sampling decision: prepends `sampling.priority` key-value pair + `sampling.score` attribute to attributes collection of to-be-created span + - regardless of sampling decision: prepends `sampling.score` key-value pair into tracestate of to-be-created span - Downstream service continues a trace but has different sampling rate (it's - also configured to use `ExternalPrioritySampler`) - - `ExternalPrioritySampler.ShouldSample` checks if priority is provided in + also configured to use `ExternalScoreSampler`) + - `ExternalScoreSampler.ShouldSample` checks if score is provided in `tracestate`. - - makes sampling decision by comparing upstream-generated priority with its + - makes sampling decision by comparing upstream-generated score with its sampling rate - - if span will be recorded: sampler adds `sampling.priority` attribute to + - if span will be recorded: sampler adds `sampling.score` attribute to attributes collection of to-be-created span Here is a [proof of concept](https://github.com/lmolkova/opentelemetry-dotnet/pull/1) @@ -109,9 +112,9 @@ in .NET. 1. Add `SamplingResult.Tracestate` field: sampler should be able to assign a new tracestate for to-be-created span -2. Add convention for `sampling.priority` attribute on span (TBC). Check out +2. Add convention for `sampling.score` attribute on span (TBC). Check out [open questions](open-questions) regarding attribute vs special field. -3. Add `ExternalPrioritySampler` implementation of `Sampler` +3. Add `ExternalScoreSampler` implementation of `Sampler` ### Trade-offs and mitigations @@ -124,34 +127,41 @@ Customers should configure it explicitly to avoid the overhead in the default case when interoperability is not necessary. Vendors may gradually update their existing solutions to support external -priority in order to interoperate with OpenTelemetry and should recommend +score in order to interoperate with OpenTelemetry and should recommend customers to configure such sampler. It may be the case that after migration to OpenTelemetry is finalized, the need -of `sampling.priority` will decrease and customers can remove -`ExternalPrioritySampler` from configuration. +of `sampling.score` will decrease and customers can remove +`ExternalScoreSampler` from configuration. ## Prior art and alternatives -[OpenTelemetry collector](https://github.com/open-telemetry/opentelemetry-collector/blob/60b03d0d2d503351501291b30836d2126487a741/processor/samplingprocessor/probabilisticsamplerprocessor/testdata/config.yaml#L10) uses `sampling.priority` -[OpenTracing](https://github.com/opentracing/specification/blob/master/semantic_conventions.md) -defined implementation-specific hint for sampler to prioritize recording a span. Related discussions on [Probability sampler](https://github.com/open-telemetry/opentelemetry-specification/pull/570) +### Sampling.score is NOT priority + +Priority is used by [OpenTracing](https://github.com/opentracing/specification/blob/master/semantic_conventions.md) +as an implementation-specific hint for sampler to prioritize recording a span. + +[OpenTelemetry collector](https://github.com/open-telemetry/opentelemetry-collector/blob/60b03d0d2d503351501291b30836d2126487a741/processor/samplingprocessor/probabilisticsamplerprocessor/testdata/config.yaml#L10) +uses `sampling.priority` to hint collector's sampler decision + +To avoid conflicts with existing implementations we fo not reuse priority term. + ## Open questions -### Priority calculation: can we use ProbabilitySampler? +### Score calculation: can we use ProbabilitySampler? -This spec suggests to generate priority randomly to achieve uniform +This spec suggests to generate score randomly to achieve uniform distribution. Assuming trace-ids are uniformly distributed, `ProbabilitySampler` can generate -priority, so the flow could look like this: +score, so the flow could look like this: -`ExternalPrioritySampler.ShouldSample`: +`ExternalScoreSampler.ShouldSample`: -- checks if `sampling.priority` is available in the tracestate -- if it's not there, invokes `ProbabilitySampler`, which calculates priority +- checks if `sampling.score` is available in the tracestate +- if it's not there, invokes `ProbabilitySampler`, which calculates score and populates it on the attributes - updates tracestate @@ -164,14 +174,14 @@ calculation algorithm was used. #### Cons - There is no requirement for trace-ids to be uniformly distributed -- No clear boundary between `ProbabilitySampler` and `ExternalPrioritySampler`. -`ProbabilitySampler` needs to set priority in attribute even if there is no -`ExternalPrioritySampler`. +- No clear boundary between `ProbabilitySampler` and `ExternalScoreSampler`. +`ProbabilitySampler` needs to set score in attribute even if there is no +`ExternalScoreSampler`. ### Attribute vs field on the span to-be-created Collection of attributes which is passed to sampler is empty by default to -minimize perf impact. Propagating priority back from sampler to span requires +minimize perf impact. Propagating score back from sampler to span requires to initialize the collection. Creating a new float field on `SamplingDecision` could be an alternative. @@ -189,7 +199,7 @@ fields exporters may need. ``` struct SamplingInfo - Priority, + Score, Rate/Count, ... ``` From a14a7d4f3c9f1bfd1c49ad692fbb87cf4d255a38 Mon Sep 17 00:00:00 2001 From: Liudmila Date: Fri, 21 Aug 2020 09:02:48 -0700 Subject: [PATCH 16/23] minor fixes --- text/trace/0107-sampling-priority.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index 2092ccfb4..5ad9a0637 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -5,15 +5,16 @@ sampling rates and probability calculation algorithms. ## TL;DR -**Score** in the scope of this spec is a floating point number associated with +**Score** is a floating point number associated with the trace. It's calculated when trace starts and flows in the `tracestate`, it's used by samplers to make consistent sampling decisions. -*Score* is not related to sampling *rate* (aka *probability* - sampler's -configuration not specific to trace). + +*Score* is not related to sampling *rate* (aka *probability* which represents +sampler's configuration not specific to trace). Service that starts the trace calculates the score and adds it to the `tracestate` so downstream services can re-use it to make their sampling -decisions instead of re-calculating score as a function of trace-id +decisions *instead of* re-calculating score as a function of trace-id (or trace-flags). ## Motivation From 197b1c1dd9484b63dce1405609bccdd67f360d46 Mon Sep 17 00:00:00 2001 From: Liudmila Date: Fri, 21 Aug 2020 09:03:42 -0700 Subject: [PATCH 17/23] minor fixes --- text/trace/0107-sampling-priority.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-priority.md index 5ad9a0637..8655cd9ae 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-priority.md @@ -9,14 +9,14 @@ sampling rates and probability calculation algorithms. the trace. It's calculated when trace starts and flows in the `tracestate`, it's used by samplers to make consistent sampling decisions. -*Score* is not related to sampling *rate* (aka *probability* which represents -sampler's configuration not specific to trace). - Service that starts the trace calculates the score and adds it to the `tracestate` so downstream services can re-use it to make their sampling decisions *instead of* re-calculating score as a function of trace-id (or trace-flags). +*Score* is not related to sampling *rate* (aka *probability* which represents +sampler's configuration not specific to trace). + ## Motivation The goal is to enable a mechanism for consistent (best effort) sampling From 488ae173ba6b8c84fd42ab2124eaf781551df3fc Mon Sep 17 00:00:00 2001 From: Liudmila Date: Fri, 21 Aug 2020 10:35:06 -0700 Subject: [PATCH 18/23] rename file --- .../{0107-sampling-priority.md => 0107-sampling-score.md} | 6 ++++++ 1 file changed, 6 insertions(+) rename text/trace/{0107-sampling-priority.md => 0107-sampling-score.md} (96%) diff --git a/text/trace/0107-sampling-priority.md b/text/trace/0107-sampling-score.md similarity index 96% rename from text/trace/0107-sampling-priority.md rename to text/trace/0107-sampling-score.md index 8655cd9ae..652d37fef 100644 --- a/text/trace/0107-sampling-priority.md +++ b/text/trace/0107-sampling-score.md @@ -208,4 +208,10 @@ struct SamplingInfo `SamplingResult` would allow sampler for fill it for the span-to-be-created. `Span` and its exportable representations will also need to be updated. +### Tracestate type on the SamplingResult + +By default it makes sense to propagate tracestate [blindly](https://github.com/open-telemetry/opentelemetry-specification/issues/478) +as a string for perf reasons. Specification does not define `Tracestate` type +and leaves it up to the implementation. + ## Future possibilities From ed978c7ca12cca747d84f640057ed0b822cefe88 Mon Sep 17 00:00:00 2001 From: Liudmila Date: Fri, 21 Aug 2020 15:52:23 -0700 Subject: [PATCH 19/23] lint --- text/trace/0107-sampling-score.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/trace/0107-sampling-score.md b/text/trace/0107-sampling-score.md index 652d37fef..f6b9991f8 100644 --- a/text/trace/0107-sampling-score.md +++ b/text/trace/0107-sampling-score.md @@ -6,7 +6,7 @@ sampling rates and probability calculation algorithms. ## TL;DR **Score** is a floating point number associated with -the trace. It's calculated when trace starts and flows in the `tracestate`, +the trace. It's calculated when trace starts and flows in the `tracestate`, it's used by samplers to make consistent sampling decisions. Service that starts the trace calculates the score and adds it to the From 234ddf0549c894668c75b504c2ae473cd1e77bbc Mon Sep 17 00:00:00 2001 From: Liudmila Date: Wed, 2 Sep 2020 12:59:50 -0700 Subject: [PATCH 20/23] separate score generation from sampling and other review comments --- text/trace/0107-sampling-score.md | 104 +++++++++++++++--------------- 1 file changed, 51 insertions(+), 53 deletions(-) diff --git a/text/trace/0107-sampling-score.md b/text/trace/0107-sampling-score.md index f6b9991f8..8567034cd 100644 --- a/text/trace/0107-sampling-score.md +++ b/text/trace/0107-sampling-score.md @@ -5,17 +5,21 @@ sampling rates and probability calculation algorithms. ## TL;DR -**Score** is a floating point number associated with -the trace. It's calculated when trace starts and flows in the `tracestate`, -it's used by samplers to make consistent sampling decisions. +**Score** is a floating point number associated with the trace. +It's calculated when trace starts and flows in the `tracestate`. + +*Score* is independent of sampling *probability* (aka *rate*) which represents +sampler's configuration, not specific to trace. + +Sampler can compare the *score* with the configured *probability* to make +sampling decisions. Service that starts the trace calculates the score and adds it to the `tracestate` so downstream services can re-use it to make their sampling decisions *instead of* re-calculating score as a function of trace-id -(or trace-flags). - -*Score* is not related to sampling *rate* (aka *probability* which represents -sampler's configuration not specific to trace). +(or trace-flags). This allows to configure sampling algorithm on the first +service ans avoid coordination of algorithms when multiple tracing tools are +involved. ## Motivation @@ -23,15 +27,16 @@ The goal is to enable a mechanism for consistent (best effort) sampling between services with different sampling rates and different probability calculation algorithms (for interoperability with existing tracing tools). -Consistent sampling decision made in each app of a distributed trace is -important for better user experience of trace analysis. Consistency is achieved -by following means: +Today consistency across multiple services is achieved by following means: -1. Same hashing algorithms used across all apps in a trace. - Coordination of sampling algorithms across multiple apps not always possible: - for example existing components in a system use vendor-specific - tracing tool (pre-OpenTelemetry and update is hard to justify) while there - is a desire to use OpenTelemetry for new components. +1. Same hashing algorithms on trace-id applied on each span. + Problems: + - **same sampling algorithm must be used across multiple apps**: it is + not always possible e.g. when existing components in a system use + vendor-specific tracing tool (pre-OpenTelemetry and major upgrade is hard to + justify) while new components are instrumented with OpenTelemetry. + - **trace-id uniform distribution is not guaranteed** therefore sampling + decisions could be biased 2. Sampling flag propagated from the head component/app is used by downstream apps to sample in a given trace. @@ -40,16 +45,22 @@ by following means: ## Explanation -Sampling propabaility is generated by the first service to make sampling -decision. It's a random float number in [0, 1] range. +Sampling score is generated by the first service to make sampling +decision. It's a random float (6-9 digits precision) number in [0, 1] range. Score is stamped on the span and also propagated further within `tracestate`. Next service reads score from `tracestate` (instead of calculating it from trace-id) and compares it with its sampling rate to make sampling decision. -Score is also exposed through span attributes. Vendors can leverage it +Score is exposed through span attributes. Vendors can leverage it to sort traces based on their completeness: the lower the value of score is, -the higher the chance it was sampled it by each component. +the higher the chance it was sampled in by each component. + +Vendors can enable interoperability (in terms of sampling) between legacy +tools and OpenTelemetry: legacy libraries can be updated in non-breaking way to +support external score sampling. Updating current vendor-specific library +version on the existing service in a backward-compatible way is much easier +than upgrading to OpenTelemetry. ### Example @@ -91,7 +102,8 @@ Vendors can pick the most complete traces sorting them by score. - Service that starts a trace makes sampling decision. It's configured to use `ExternalScoreSampler`(name TBD) is configured by user. Within `ShouldSample` callback sampler - - generates random float score (6-9 digits) in [0, 1] interval + - generates score [0, 1] interval using `SamplingScoreGenerator` that can run + random or deterministic `hash(trace-id)` algorithm. - makes sampling decision by comparing generated score to configured rate - if decision is `RECORD` (or `RECORD_AND_SAMPLED`), sampler adds `sampling.score` attribute to attributes collection of to-be-created span @@ -105,17 +117,25 @@ callback sampler sampling rate - if span will be recorded: sampler adds `sampling.score` attribute to attributes collection of to-be-created span +- If downstream service does not find a score in the tracestate, it falls back + to the configured score generation algorithm and updates tracestate and + attributes Here is a [proof of concept](https://github.com/lmolkova/opentelemetry-dotnet/pull/1) in .NET. ### Specification Delta -1. Add `SamplingResult.Tracestate` field: sampler should be able to assign a - new tracestate for to-be-created span -2. Add convention for `sampling.score` attribute on span (TBC). Check out +1. Add `SamplingResult.Tracestate` field: sampler should be able to [assign a + new tracestate for to-be-created span](https://github.com/open-telemetry/opentelemetry-specification/issues/856) +2. Add convention for `sampling.score` attribute on span (TBD). Check out [open questions](open-questions) regarding attribute vs special field. -3. Add `ExternalScoreSampler` implementation of `Sampler` +3. Add notion of `SamplingScoreGenerator` that has `TraceIdRatioGenerator`, + `RandomGenerator`, etc implementations. + - Change `TraceIdRatioBased` sampler to use corresponding generator and serve + as generic probability sampler with configurable score generation approach. +4. Add `ExternalScoreSampler` implementation of `Sampler`. It's created with + probability value and implementation of `SamplingScoreGenerator`. ### Trade-offs and mitigations @@ -147,37 +167,15 @@ as an implementation-specific hint for sampler to prioritize recording a span. [OpenTelemetry collector](https://github.com/open-telemetry/opentelemetry-collector/blob/60b03d0d2d503351501291b30836d2126487a741/processor/samplingprocessor/probabilisticsamplerprocessor/testdata/config.yaml#L10) uses `sampling.priority` to hint collector's sampler decision -To avoid conflicts with existing implementations we fo not reuse priority term. +To avoid conflicts with existing implementations we do not reuse priority term. ## Open questions -### Score calculation: can we use ProbabilitySampler? - -This spec suggests to generate score randomly to achieve uniform -distribution. - -Assuming trace-ids are uniformly distributed, `ProbabilitySampler` can generate -score, so the flow could look like this: - -`ExternalScoreSampler.ShouldSample`: - -- checks if `sampling.score` is available in the tracestate -- if it's not there, invokes `ProbabilitySampler`, which calculates score - and populates it on the attributes -- updates tracestate - -#### Pros - -Fallback to `ProbabilitySampler` improves the case when `tracestate` is trimmed -so there is a chance sampling could be consistent if same probability -calculation algorithm was used. - -#### Cons +### Should we separate sampling from score generation? -- There is no requirement for trace-ids to be uniformly distributed -- No clear boundary between `ProbabilitySampler` and `ExternalScoreSampler`. -`ProbabilitySampler` needs to set score in attribute even if there is no -`ExternalScoreSampler`. +Rate-based sampling in this spec is separated from score generation. Sampler can +be configured to use any algorithm on sampling parameters. Different samplers +may reuse generation algorithms. ### Attribute vs field on the span to-be-created @@ -189,8 +187,8 @@ Creating a new float field on `SamplingDecision` could be an alternative. It'd also require adding similar property on Span/SpanData. There are other scenarios when sampling information is useful for -exporter: e.g. sampling rate (or inverse value: count of spans -this span represent). Exporters can use it to estimate metrics. +exporter: e.g. sampling rate (or it's inverse value: count of spans +this span represents), exporters can use it to estimate metrics. Populating all sampling information on all spans may be inefficient in terms of event payload size and storage while being useful for a subset of vendors. From b7a204062d42ed75ea28750c0d236c8457d04912 Mon Sep 17 00:00:00 2001 From: Liudmila Date: Wed, 2 Sep 2020 13:33:40 -0700 Subject: [PATCH 21/23] more review comments --- text/trace/0107-sampling-score.md | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/text/trace/0107-sampling-score.md b/text/trace/0107-sampling-score.md index 8567034cd..6770dc853 100644 --- a/text/trace/0107-sampling-score.md +++ b/text/trace/0107-sampling-score.md @@ -120,6 +120,19 @@ callback sampler - If downstream service does not find a score in the tracestate, it falls back to the configured score generation algorithm and updates tracestate and attributes +- Any service can be configured to use other samplers (e.g. `TraceIdRatioBased`) + In this case, score in tracestate is not affecting sampling decisions and is + re-calculated by sampler. + +`ExternalScoreSampler` is responsible for: + +- reading and writing score to the `tracestate` +- if score is set on the tracestate it makes sampling decision +- if score is not present, it generates one using `SamplingScoreGenerator`. + +`SamplingScoreGenerator` responsible for: + +- calculating score in random or deterministic way based on sampling parameters. Here is a [proof of concept](https://github.com/lmolkova/opentelemetry-dotnet/pull/1) in .NET. @@ -130,8 +143,10 @@ in .NET. new tracestate for to-be-created span](https://github.com/open-telemetry/opentelemetry-specification/issues/856) 2. Add convention for `sampling.score` attribute on span (TBD). Check out [open questions](open-questions) regarding attribute vs special field. -3. Add notion of `SamplingScoreGenerator` that has `TraceIdRatioGenerator`, - `RandomGenerator`, etc implementations. +3. Add notion of `SamplingScoreGenerator` that is capable of calculating float + score from sampling parameters. + It has `TraceIdRatioGenerator`, `RandomGenerator` and possible other + implementations. - Change `TraceIdRatioBased` sampler to use corresponding generator and serve as generic probability sampler with configurable score generation approach. 4. Add `ExternalScoreSampler` implementation of `Sampler`. It's created with @@ -157,6 +172,8 @@ of `sampling.score` will decrease and customers can remove ## Prior art and alternatives +[TraceIdRatioBased](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#traceidratiobased) sampler. + Related discussions on [Probability sampler](https://github.com/open-telemetry/opentelemetry-specification/pull/570) ### Sampling.score is NOT priority From 7c0646934b09d435e6942f9220cb99dbf46b734f Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Mon, 21 Sep 2020 15:51:05 -0700 Subject: [PATCH 22/23] Reference float standard --- text/trace/0107-sampling-score.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/text/trace/0107-sampling-score.md b/text/trace/0107-sampling-score.md index 6770dc853..420a22338 100644 --- a/text/trace/0107-sampling-score.md +++ b/text/trace/0107-sampling-score.md @@ -46,7 +46,8 @@ Today consistency across multiple services is achieved by following means: ## Explanation Sampling score is generated by the first service to make sampling -decision. It's a random float (6-9 digits precision) number in [0, 1] range. +decision. It's a random float (6-9 digits precision, IEEE-754 32-bit +floating-point) number in [0, 1] range. Score is stamped on the span and also propagated further within `tracestate`. Next service reads score from `tracestate` (instead of calculating it from From 0721be603a8f80bc7c1a648957e5c3b5f04fcb11 Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Tue, 29 Sep 2020 09:29:11 -0700 Subject: [PATCH 23/23] Spec now allows samplers to modify tracestate Incorporating changes from https://github.com/open-telemetry/opentelemetry-specification/pull/988 --- text/trace/0107-sampling-score.md | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/text/trace/0107-sampling-score.md b/text/trace/0107-sampling-score.md index 420a22338..bcbeea9b0 100644 --- a/text/trace/0107-sampling-score.md +++ b/text/trace/0107-sampling-score.md @@ -140,17 +140,15 @@ in .NET. ### Specification Delta -1. Add `SamplingResult.Tracestate` field: sampler should be able to [assign a - new tracestate for to-be-created span](https://github.com/open-telemetry/opentelemetry-specification/issues/856) -2. Add convention for `sampling.score` attribute on span (TBD). Check out +1. Add convention for `sampling.score` attribute on span (TBD). Check out [open questions](open-questions) regarding attribute vs special field. -3. Add notion of `SamplingScoreGenerator` that is capable of calculating float +2. Add notion of `SamplingScoreGenerator` that is capable of calculating float score from sampling parameters. It has `TraceIdRatioGenerator`, `RandomGenerator` and possible other implementations. - Change `TraceIdRatioBased` sampler to use corresponding generator and serve as generic probability sampler with configurable score generation approach. -4. Add `ExternalScoreSampler` implementation of `Sampler`. It's created with +3. Add `ExternalScoreSampler` implementation of `Sampler`. It's created with probability value and implementation of `SamplingScoreGenerator`. ### Trade-offs and mitigations @@ -223,11 +221,3 @@ struct SamplingInfo `SamplingResult` would allow sampler for fill it for the span-to-be-created. `Span` and its exportable representations will also need to be updated. - -### Tracestate type on the SamplingResult - -By default it makes sense to propagate tracestate [blindly](https://github.com/open-telemetry/opentelemetry-specification/issues/478) -as a string for perf reasons. Specification does not define `Tracestate` type -and leaves it up to the implementation. - -## Future possibilities