Add probability to decision when appropriate #1116

jkwatson · 2020-04-15T21:40:46Z

Resolves #1087

Does this look like what you're looking for, @pavolloffay ?

Oberon00

IMHO, this must go through spec first, otherwise we risk coming up with incompatible definitions for these attributes relative to other languages.

sdk/src/main/java/io/opentelemetry/sdk/trace/Samplers.java

thisthat · 2020-04-16T10:59:17Z

You might add an entry in the SemanticAttributes class and use that. This way, if in the spec we agree to a different name, the update will be straight forward

sdk/src/main/java/io/opentelemetry/sdk/trace/Samplers.java

…ributes

bogdandrutu · 2020-04-16T15:38:48Z

api/src/main/java/io/opentelemetry/trace/attributes/SemanticAttributes.java

@@ -125,5 +125,9 @@
  /** JDBC substring like "mysql://db.example.com:3306" */
  public static final StringAttributeSetter DB_URL = StringAttributeSetter.create("db.url");

+  /** Probability value used by a probability-based Span sampling strategy. */
+  public static final DoubleAttributeSetter SAMPLING_PROBABILITY =


I would put this in the SDK since it is SDK specific.

It's probably also useful for vendor API implementations though.

Can you give an example of how an API user would want to use this attribute?

@jkwatson this is for the moment only in java so we cannot have it here. I would suggest two things:

Open a PR in specs to add this

Move it as internal member in probability sampler for the moment. and document that we need to follow the specs.

Yes, strong agree. I'll make it happen.

I see. Do you think there's a whole class of stuff that an alternate SDK implementation would want to use from the default SDK?

Even if that is the case, I am not sure we can have this in the API: Sampler is not part of the API (and explicitly removed from the API any "sampling" logic).

So I think this should be moved to SDK anyway.

I was just wondering if there was going to be a need for some sort of "SDK Support" module that could contain this kind of thing. I'm not suggesting we do that now, though. :)

open-telemetry/opentelemetry-specification#570 for the spec change

ok, moved into the SDK. PTAL, @bogdandrutu

sdk/src/main/java/io/opentelemetry/sdk/trace/Samplers.java

bogdandrutu · 2020-04-20T18:04:09Z

api/src/main/java/io/opentelemetry/trace/attributes/SemanticAttributes.java

@@ -125,5 +125,9 @@
  /** JDBC substring like "mysql://db.example.com:3306" */
  public static final StringAttributeSetter DB_URL = StringAttributeSetter.create("db.url");

+  /** Probability value used by a probability-based Span sampling strategy. */
+  public static final DoubleAttributeSetter SAMPLING_PROBABILITY =


Even if that is the case, I am not sure we can have this in the API: Sampler is not part of the API (and explicitly removed from the API any "sampling" logic).

So I think this should be moved to SDK anyway.

codecov-io · 2020-04-20T20:41:25Z

Codecov Report

Merging #1116 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1116      +/-   ##
============================================
+ Coverage     85.68%   85.73%   +0.05%     
- Complexity     1081     1082       +1     
============================================
  Files           138      138              
  Lines          3974     3981       +7     
  Branches        351      351              
============================================
+ Hits           3405     3413       +8     
+ Misses          427      426       -1     
  Partials        142      142

Impacted Files	Coverage Δ	Complexity Δ
...main/java/io/opentelemetry/sdk/trace/Samplers.java	`89.13% <100.00%> (+1.95%)`	`4.00 <0.00> (ø)`
...ava/io/opentelemetry/sdk/trace/SpanBuilderSdk.java	`93.13% <100.00%> (ø)`	`39.00 <0.00> (ø)`
...emetry/trace/attributes/DoubleAttributeSetter.java	`55.55% <0.00%> (+11.11%)`	`3.00% <0.00%> (+1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adf82e7...94ab8f5. Read the comment docs.

bogdandrutu · 2020-04-20T22:42:58Z

sdk/src/main/java/io/opentelemetry/sdk/trace/SamplingAttributes.java

+  public static final DoubleAttributeSetter SAMPLING_PROBABILITY =
+      DoubleAttributeSetter.create("sampling.probability");


As I mentioned in a previous comment, I would make this internal in the Samplers until a decision in the specs is made. I think this was the confusion from the other thread, but would not expose it for the moment, my comment there was that if we decide to expose it will definitely not going to be in the API.

oh yeah. I somehow spaced on that part of things. I'll move it in there for the nonce.

Thanks, please do it before merging :)

Also good to wait for @Oberon00 because they requested changes.

moved into the Samplers, near the usage.

bogdandrutu · 2020-04-20T23:01:58Z

sdk/src/main/java/io/opentelemetry/sdk/trace/SamplingAttributes.java

+  public static final DoubleAttributeSetter SAMPLING_PROBABILITY =
+      DoubleAttributeSetter.create("sampling.probability");


Thanks, please do it before merging :)

bogdandrutu · 2020-04-20T23:03:51Z

@Oberon00 you requested changes. Are you ok with moving forward with this being an internal member for the moment until specs are written?

bogdandrutu · 2020-04-22T15:18:01Z

@Oberon00 ping :)

pavolloffay · 2020-04-23T06:39:06Z

Thanks for the PR @jkwatson

* add the sampling probability to the decision, when appropriate. * formatting * normalize a method name and move attribute to semantic convention attributes * move the sampling attribute to its own class in the SDK * Move the sampling priority attribute to a non-public spot

add the sampling probability to the decision, when appropriate.

0e79d16

jkwatson requested review from arminru, bogdandrutu, carlosalberto, pavolloffay, songy23 and tylerbenson as code owners April 15, 2020 21:40

formatting

9b0e502

thisthat approved these changes Apr 16, 2020

View reviewed changes

Oberon00 suggested changes Apr 16, 2020

View reviewed changes

Oberon00 reviewed Apr 16, 2020

View reviewed changes

sdk/src/main/java/io/opentelemetry/sdk/trace/Samplers.java Outdated Show resolved Hide resolved

bogdandrutu reviewed Apr 16, 2020

View reviewed changes

sdk/src/main/java/io/opentelemetry/sdk/trace/Samplers.java Outdated Show resolved Hide resolved

sdk/src/main/java/io/opentelemetry/sdk/trace/Samplers.java Outdated Show resolved Hide resolved

normalize a method name and move attribute to semantic convention att…

204b4d3

…ributes

bogdandrutu reviewed Apr 16, 2020

View reviewed changes

bogdandrutu reviewed Apr 20, 2020

View reviewed changes

move the sampling attribute to its own class in the SDK

8ffc590

bogdandrutu approved these changes Apr 20, 2020

View reviewed changes

Move the sampling priority attribute to a non-public spot

94ab8f5

arminru approved these changes Apr 22, 2020

View reviewed changes

Oberon00 approved these changes Apr 22, 2020

View reviewed changes

bogdandrutu merged commit 4d62068 into open-telemetry:master Apr 22, 2020

jkwatson added this to the May Release milestone May 1, 2020

Oberon00 mentioned this pull request Jan 4, 2021

Remove off-spec sampling.probability from TraceIdRatioBasedSampler attribute (for now) #2420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add probability to decision when appropriate #1116

Add probability to decision when appropriate #1116

jkwatson commented Apr 15, 2020

Oberon00 left a comment

thisthat commented Apr 16, 2020

bogdandrutu Apr 16, 2020

Oberon00 Apr 16, 2020

jkwatson Apr 16, 2020

bogdandrutu Apr 17, 2020

jkwatson Apr 17, 2020

jkwatson Apr 20, 2020

bogdandrutu Apr 20, 2020

jkwatson Apr 20, 2020

jkwatson Apr 20, 2020

jkwatson Apr 20, 2020

bogdandrutu Apr 20, 2020

codecov-io commented Apr 20, 2020 •

edited

Loading

bogdandrutu Apr 20, 2020

jkwatson Apr 20, 2020

bogdandrutu Apr 20, 2020

bogdandrutu Apr 20, 2020

jkwatson Apr 20, 2020 •

edited

Loading

bogdandrutu Apr 20, 2020

bogdandrutu commented Apr 20, 2020

bogdandrutu commented Apr 22, 2020

pavolloffay commented Apr 23, 2020

		public static final DoubleAttributeSetter SAMPLING_PROBABILITY =
		DoubleAttributeSetter.create("sampling.probability");

Add probability to decision when appropriate #1116

Add probability to decision when appropriate #1116

Conversation

jkwatson commented Apr 15, 2020

Oberon00 left a comment

Choose a reason for hiding this comment

thisthat commented Apr 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 20, 2020 • edited Loading

Codecov Report

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

jkwatson Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Apr 20, 2020

bogdandrutu commented Apr 22, 2020

pavolloffay commented Apr 23, 2020

codecov-io commented Apr 20, 2020 •

edited

Loading

jkwatson Apr 20, 2020 •

edited

Loading