Store sampling.probability in sampled span attributes

[pmm-sumo]

Description:

Adds sampling.probability attribute to all spans sampled by probabilisticsamplerprocessor.

This makes it consistent with Java probabilisitic sampler design and the ongoing specification update

Probably, if this is going to be merged then after the specification PR gets accepted

Link to tracking Issue: Related specification change open-telemetry/opentelemetry-specification#570

Testing: Unit tests added

Documentation: README.md updated with description of attribute added

cc @Oberon00

[pmm-sumo]

Since the linked PR is currently neither required-for-ga nor allowed-for-ga I am converting it to draft for the time being to not interfere with GA progress

[codecov]

Codecov Report

Merging #2215 (ae407ee) into master (47ffc51) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2215      +/-   ##
==========================================
- Coverage   91.95%   91.92%   -0.03%     
==========================================
  Files         271      271              
  Lines       15664    15658       -6     
==========================================
- Hits        14404    14394      -10     
- Misses        857      860       +3     
- Partials      403      404       +1     

Impacted Files	Coverage Δ
translator/conventions/opentelemetry.go	100.00% <ø> (ø)
...babilisticsamplerprocessor/probabilisticsampler.go	92.03% <100.00%> (+0.52%)	⬆️
internal/data/traceid.go	84.61% <0.00%> (-7.70%)	⬇️
exporter/exporterhelper/metricshelper.go	94.59% <0.00%> (-5.41%)	⬇️
exporter/otlpexporter/otlp.go	71.79% <0.00%> (-2.57%)	⬇️

---

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 47ffc51...ae407ee. Read the comment docs.

[bogdandrutu]

Is this really a multiplication here? I think it depends on the algorithm used, may be a min or multiplication.

[pmm-sumo]

I think this problem should be somewhat mitigated by this: https://github.com/open-telemetry/opentelemetry-collector/blob/master/processor/samplingprocessor/probabilisticsamplerprocessor/probabilisticsampler.go#L110-L115

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[Oberon00]

@jkwatson I guess the probability attribute should be removed from Java for now, as it seems there is no agreement on adding it.

[pmm-sumo]

@jkwatson I guess the probability attribute should be removed from Java for now, as it seems there is no agreement on adding it.

@Oberon00 I think both this PR and the Spec PR were closed due to lack of recent activity rather than lack of agreement. Also, I think the discussion under the Spec PR mostly touches "how" of doing probability sampling rather than the need for sampling.probability tag (or lack of thereof). From my point of view - it is very usable for the backend. Perhaps worth bringing up during the next Spec SIG? Or reviving Sampling WG? (Even if that would be post-GA for tracing)

[Oberon00]

I didn't mean to imply that people are against it, I don't think that's the case. The missing agreement (in form of approving reviews and a merge) seems to be due to lack of interest relative to other things currently in progress.

And I actually wanted to comment on the related spec PR which was also closed due to inactivity open-telemetry/opentelemetry-specification#570, but it kinda fits here too I guess 😃

[pmm-sumo]

@Oberon00 yeah, it's all towards getting the GA out and I agree that this might not be above the cutoff line of what's really important and what's not 😀