update ShouldSample's parent parameter to be Context

[drexler]

Resolves #290

[codecov]

Codecov Report

Merging #368 (65c7c75) into master (ea61575) will increase coverage by 0.02%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
+ Coverage   53.90%   53.92%   +0.02%     
==========================================
  Files          71       71              
  Lines        6074     6081       +7     
==========================================
+ Hits         3274     3279       +5     
- Misses       2800     2802       +2     

Impacted Files	Coverage Δ
opentelemetry/src/sdk/trace/sampler.rs	86.13% <60.00%> (-0.74%)	⬇️
opentelemetry/src/sdk/trace/tracer.rs	71.92% <100.00%> (+0.84%)	⬆️
...ntelemetry/src/sdk/metrics/aggregators/ddsketch.rs	76.76% <0.00%> (-0.20%)	⬇️

---

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 ea61575...65c7c75. Read the comment docs.

[jtescher]

So looking further into the discussion in open-telemetry/opentelemetry-specification#881, it looks like the intent may be to have access to the parent span itself from the context. May have to think about this more as currently Extractors product a context with a remote span context, but not a remote span. The current patch will technically be in accordance with the api in the spec, but it is effectively still only exposing the span context.

[TommyCpp]

currently Extractors product a context with a remote span context, but not a remote span

I think it would be wired to build a sdk::Span for a remote span because in that case it's hard to assign a tracer to it. Maybe just set the remote span context in context and pass it into Sampler?

[jtescher]

@TommyCpp yeah that seems similar to what we currently have right? Not sure the best way to apply the spec change if they do want to have access to full spans, also an option to simply not conform to the spec for this language.

[TommyCpp]

I think at API level, passing Context gives the potential to determine whether to sample based on all available information in Context. That make sense because the Extractor can put anything into the Context.

But for SDK I guess we are only use the SpanContext to determine whether to sample. That could come from either remote span or local parent span. Could we have some logic that check for Span in Context first, and if it's not available, we could use RemoteSpanContext?

[jtescher]

Any preferences for the API here @awiede / @frigus02?

[frigus02]

Sorry, personal life is a bit busy at the moment. I don't manage to do as much on the project. I hope that changes in a week or two.

On topic: I think you're right. The purpose of this change seems to be to have the entire parent span available.

It sounds like this requires additional changes anyway, like always accepting a Context to specify the parent. With that in mind, I think I would be happy with the PR. I would hope that this API can stay.

I didn't yet think about if it's feasible to implement these changes, though. So I'm okay with going in another direction, too.

[TommyCpp]

I don't think we need to change API even if we want to do a subsequent PR. So I guess we can merge it as it is.

[jtescher]

I think this can be merged with one small adjustment below, even if the intention of the spec is not followed, then as @TommyCpp mentioned it will at least not be a future breaking change. The performance changes look negligible from the trace benches so no real downside.

[drexler]

Thanks for the review and feedback. I'll take a look wrap this up come the weekend.

[jtescher]

Looks good, thanks! we can work on the best ways of getting access to what is intended in the spec in subsequent PRs