feat: add OTEL_SAMPLING_PROBABILITY env var #1069

naseemkullah · 2020-05-18T15:44:08Z

Allows user to configure tracer's sampling probability via env var

Which problem is this PR solving?

JS implementation of OTEL_SAMPLING_PROBABILITY opentelemetry-specification#567

Short description of the changes

Tracer.ts looks to see if process.env.OTEL_SAMPLING_PROBABILITY is set and if so determines the sampling probability by converting its contents to a number and passing it to the ProbabilitySampler constructor. If unset it defaults to passing in a probability of 1.

dyladan · 2020-05-18T15:44:39Z

Looks like somehow CI might have been disabled?

naseemkullah · 2020-05-18T15:44:49Z

Anyone know why CI is not being triggered?

naseemkullah · 2020-05-18T15:45:39Z

Looks like somehow CI might have been disabled?

Yes I've noticed this for all my open PRs. 🤔 It's a conspiracy I tell you! 😛

dyladan · 2020-05-18T15:46:22Z

CI ran in #1063 less than an hour ago

naseemkullah · 2020-05-18T15:47:29Z

CI ran in #1063 less than an hour ago

I think it's for my PRs in particular. Did I consume too much build time with my obnoxious amount of pushes? 😆

dyladan · 2020-05-18T15:47:54Z

I don't think it works per-user like that.

obecny · 2020-05-18T15:51:48Z

It is a punishment from github for too many force push :D

packages/opentelemetry-tracing/src/Tracer.ts

dyladan · 2020-05-18T15:57:12Z

https://support.circleci.com/hc/en-us/articles/360008097173-Why-aren-t-pull-requests-triggering-jobs-on-my-organization-?auth_token=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJhY2NvdW50X2lkIjo4MjgyMDksInVzZXJfaWQiOm51bGwsInRpY2tldF9pZCI6NzI1NTMsImRlZmxlY3Rpb25faWQiOjM2MDM3NzAwOTY1MSwiYXJ0aWNsZXMiOlszNjAwMzQ1MzY0MzMsMzYwMDA4MDk3MTczLDExNTAxNDAwNTE4OF0sInRva2VuIjpudWxsLCJleHAiOjE1OTI0MDkzODl9.coNQRiX5WOjC1sG_amQgZ21Dkr3KqHhmj07mErkkfNY

dyladan · 2020-05-18T16:43:48Z

@naseemkullah can you confirm if that is actually your issue?

naseemkullah · 2020-05-18T18:34:02Z

@naseemkullah can you confirm if that is actually your issue?

Thanks @dyladan, so I had revoked the github app as I did not think I needed it (i.e. it is not used in my personal projects currently and it said last used within last 7 months or something.) I re-granted access and things are now in order!

dyladan · 2020-05-18T18:39:53Z

Yay the build failed!

naseemkullah · 2020-05-18T18:41:19Z

Yay the build failed!

You don't hear that everyday!

codecov · 2020-05-21T03:30:11Z

Codecov Report

Merging #1069 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1069   +/-   ##
=======================================
  Coverage   92.69%   92.70%           
=======================================
  Files         193      193           
  Lines        4807     4812    +5     
  Branches      983      986    +3     
=======================================
+ Hits         4456     4461    +5     
  Misses        351      351

Impacted Files	Coverage Δ
...ject/packages/opentelemetry-tracing/src/utility.ts	`100.00% <0.00%> (ø)`

packages/opentelemetry-core/src/platform/browser/sampling-probability.ts

packages/opentelemetry-core/src/platform/node/sampling-probability.ts

naseemkullah

Hi @dyladan, similarly to OTEL_LOG_LEVEL can we get this in with OTEL_SAMPLING_PROBABILITY as a working name until spec confirms the name?

packages/opentelemetry-core/src/platform/node/sampling-probability.ts

packages/opentelemetry-tracing/src/Tracer.ts

packages/opentelemetry-core/test/platform/sampling-probability.test.ts

mayurkale22 · 2020-07-09T06:28:21Z

packages/opentelemetry-tracing/src/config.ts

@@ -32,7 +32,6 @@ export const DEFAULT_MAX_LINKS_PER_SPAN = 32;
 export const DEFAULT_CONFIG = {
  defaultAttributes: {},
  logLevel: LogLevel.INFO,
-  sampler: ALWAYS_SAMPLER,


I think we should keep always sampler as default and if OTEL_SAMPLING_PROBABILITY env is set, use ProbabilitySampler. WDYT?

Also, please take a look #1279.

Sure sgtm. I'll adjust accordingly.

parentOrElse is pretty cool, today we are putting a low probability across the board, but perhaps we want to pass that low probability to a parentOrElse 🤔 🤔

Should OTEL_SAMPLING_PROBABILITY now adjust probability in the delagatedSampler of a parentOrElse sampler?? is parentOrElse always better?

OTEL_SAMPLING_PROBABILITY should probably be read by the probability sampler and the parent or else sampler. The parent or else sampler will use the always on sampler by default, but if the OTEL_SAMPLING_PROBABILITY env var exists and is the right format, it could use the probability sampler as a delegate. No need for it to play with probabilities because the probability sampler will just reread the env var.

alright, so my pending question is:

if you set OTEL_SAMPLING_PROBABILITY, (without any further config) which sampler does the tracer user? a parentOrElse or a probabilitySampler (directly)?

Oh you are saying if you have coded in the use of a parentOrElse sampler, and left the delegation empty, OTEL_SAMPLING_PROBABILITY will place a probability sampler in it?

There is no such thing as a parent or else sampler with an empty delegate

if you set OTEL_SAMPLING_PROBABILITY, (without any further config) which sampler does the tracer user? a parentOrElse or a probabilitySampler (directly)?

it should use a parent or else sampler with a probability delegate

thanks, just to confirm: parentOrElse with a prob delegate is always/generally favoured over a direct prob sampler?

Unless specifically configured otherwise by the user, yes.

naseemkullah · 2020-07-09T18:08:05Z

Does the spec hint at any implementation of metrics sampling? if so we might want to rename this to OTEL_TRACE(R)_SAMPLING_PROBABILITY

dyladan · 2020-07-09T18:18:17Z

Does the spec hint at any implementation of metrics sampling? if so we might want to rename this to OTEL_TRACE(R)_SAMPLING_PROBABILITY

Metrics aggregation takes the place of trace sampling. The point of sampling is to not overload the server. Since we don't send every metric event to the server, this is not a problem for metrics.

dyladan · 2020-07-15T13:40:35Z

@mayurkale22 are you satisfied with the updates?

@naseemkullah can you fix the conflicts? I think this can be merged soon

naseemkullah · 2020-07-16T04:07:37Z

@mayurkale22 are you satisfied with the updates?

@naseemkullah can you fix the conflicts? I think this can be merged soon

@dyladan, @mayurkale22 I've added use of ParentOrElse and I've ensured AlwaysOnSampler is used by default if sampling probability is unset or set to 1.

naseemkullah · 2020-07-16T04:22:04Z

Please let me know before merging as I'd like to clean up the history.

mayurkale22

LGTM

mayurkale22 · 2020-07-17T19:45:06Z

Please fix the build, this is good to go.

dyladan · 2020-07-16T13:11:57Z

packages/opentelemetry-tracing/src/utility.ts

+  const otelSamplingProbability = getEnv().OTEL_SAMPLING_PROBABILITY;
+  const validOtelSamplingProbability =
+    otelSamplingProbability &&
+    otelSamplingProbability >= 0 &&


you are doing number comparisons on a string here.

shoot, thanks.

actually no, its a number

but the logic pasted is slightly flawed, looking into it

I think the first condition is not needed as second otelSamplingProbability >=0 is already enough

besides that this is already checked in env so in fact OTEL_SAMPLING_PROBABILITY will always be a number between 0 and 1 (default is set to 1)

Thanks @obecny that's a good point! I will only check if it is less than one, and if not, will use the AlwaysOnSampler() as per spec's recommendation

otelSamplingProbability !== undefined && otelSamplingProbability < 1

is the check since according to ts it could be undefined.

after this PR -> #1328 ts will not complain about undefined

naseemkullah · 2020-07-18T02:42:38Z

packages/opentelemetry-tracing/src/utility.ts

+      ? {
+          sampler: new ParentOrElseSampler(delegateSampler),
+        }
+      : { sampler: DEFAULT_CONFIG.sampler },


I had to repeat the fallback/default sampler here, but not sure why (thought that {} would be good enough but no strangely).

dyladan · 2020-07-22T15:49:05Z

@naseemkullah please fix the build

Allows user to configure tracer's sampling probability via an env var. Uses parentOrElse sampler with a probability sampler delegate when setting OTEL_SAMPLING_PROBABILITY Defaults to AlwaysOnSampler when unset or set to 1. Signed-off-by: Naseem <[email protected]>

naseemkullah requested review from bg451, dyladan, markwolff, mayurkale22, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners May 18, 2020 15:44

obecny reviewed May 18, 2020

View reviewed changes

packages/opentelemetry-tracing/src/Tracer.ts Outdated Show resolved Hide resolved

mayurkale22 added the enhancement New feature or request label May 18, 2020

naseemkullah requested a review from mwear as a code owner May 21, 2020 03:30

dyladan reviewed May 21, 2020

View reviewed changes

packages/opentelemetry-core/src/platform/browser/sampling-probability.ts Outdated Show resolved Hide resolved

packages/opentelemetry-core/src/platform/node/sampling-probability.ts Outdated Show resolved Hide resolved

naseemkullah requested review from dyladan and obecny May 22, 2020 04:07

naseemkullah commented May 22, 2020

View reviewed changes

dyladan reviewed May 29, 2020

View reviewed changes

packages/opentelemetry-core/src/platform/node/sampling-probability.ts Outdated Show resolved Hide resolved

dyladan reviewed May 29, 2020

View reviewed changes

packages/opentelemetry-tracing/src/Tracer.ts Outdated Show resolved Hide resolved

markwolff approved these changes Jul 8, 2020

View reviewed changes

dyladan approved these changes Jul 8, 2020

View reviewed changes

mayurkale22 reviewed Jul 9, 2020

View reviewed changes

naseemkullah force-pushed the otel-sampling-probability branch from 3f3b9a8 to e0dd42c Compare July 9, 2020 12:44

naseemkullah force-pushed the otel-sampling-probability branch from e0dd42c to a8b7898 Compare July 16, 2020 04:05

naseemkullah force-pushed the otel-sampling-probability branch from a8b7898 to 08eef7a Compare July 16, 2020 04:11

mwear approved these changes Jul 17, 2020

View reviewed changes

mayurkale22 approved these changes Jul 17, 2020

View reviewed changes

dyladan reviewed Jul 17, 2020

View reviewed changes

naseemkullah force-pushed the otel-sampling-probability branch 3 times, most recently from 48f95ca to f40d8ad Compare July 18, 2020 02:41

naseemkullah commented Jul 18, 2020

View reviewed changes

naseemkullah force-pushed the otel-sampling-probability branch from f40d8ad to 6137226 Compare July 18, 2020 02:53

mayurkale22 added the The CircleCI build failed label Jul 20, 2020

naseemkullah force-pushed the otel-sampling-probability branch 2 times, most recently from 2c484bd to 7c50b3e Compare July 23, 2020 18:45

naseemkullah force-pushed the otel-sampling-probability branch from 7c50b3e to 6454443 Compare July 24, 2020 03:46

mayurkale22 removed the The CircleCI build failed label Jul 24, 2020

mayurkale22 merged commit 0f0c08a into open-telemetry:master Jul 24, 2020

naseemkullah deleted the otel-sampling-probability branch July 24, 2020 04:45

naseemkullah mentioned this pull request Jul 24, 2020

chore: 0.10.0 release proposal #1345

Merged

feat: add OTEL_SAMPLING_PROBABILITY env var #1069

feat: add OTEL_SAMPLING_PROBABILITY env var #1069

Conversation

naseemkullah commented May 18, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

dyladan commented May 18, 2020

naseemkullah commented May 18, 2020

naseemkullah commented May 18, 2020 • edited Loading

dyladan commented May 18, 2020

naseemkullah commented May 18, 2020

dyladan commented May 18, 2020

obecny commented May 18, 2020 • edited Loading

dyladan commented May 18, 2020

dyladan commented May 18, 2020

naseemkullah commented May 18, 2020

dyladan commented May 18, 2020

naseemkullah commented May 18, 2020

codecov bot commented May 21, 2020 • edited Loading

Codecov Report

naseemkullah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naseemkullah Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

naseemkullah Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naseemkullah commented Jul 9, 2020 • edited Loading

dyladan commented Jul 9, 2020

dyladan commented Jul 15, 2020

naseemkullah commented Jul 16, 2020

naseemkullah commented Jul 16, 2020

mayurkale22 left a comment

Choose a reason for hiding this comment

mayurkale22 commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan commented Jul 22, 2020

naseemkullah commented May 18, 2020 •

edited

Loading

naseemkullah commented May 18, 2020 •

edited

Loading

obecny commented May 18, 2020 •

edited

Loading

codecov bot commented May 21, 2020 •

edited

Loading

naseemkullah Jul 9, 2020 •

edited

Loading

dyladan Jul 9, 2020 •

edited

Loading

naseemkullah Jul 9, 2020 •

edited

Loading

naseemkullah commented Jul 9, 2020 •

edited

Loading