fix(tracing): fix HTTPPropagator sampling the wrong span when the span_context is not active

[samschlegel]

Ran into this when making a change in #13847. This feels like the right fix since we really only use the span context anyways and this will just log and error while presenting the same behavior, but this inject API feels weird. It feels like it should either just always take the span in, or somehow pull it in from the context, since there are so many if statements here to assert something that would be solved by either of those. The former feels like a breaking change however, and the latter feels bad by making SpanContext and Span circularly depend on each other, so at least logging loudly when this fails is a solid third option

Checklist

• PR author has checked that all the criteria below are met
• The PR description includes an overview of the change
• The PR description articulates the motivation for the change
• The change includes tests OR the PR description describes a testing strategy
• The PR description notes risks associated with the change, if any
• Newly-added code is easy to change
• The change follows the library release note guidelines
• The change includes or references documentation updates if necessary
• Backport labels are set (if applicable)

Reviewer Checklist

• Reviewer has checked that all the criteria below are met
• Title is accurate
• All changes are related to the pull request's stated goal
• Avoids breaking API changes
• Testing strategy adequately addresses listed risks
• Newly-added code is easy to change
• Release note makes sense to a user of the library
• If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
• Backport labels are set in a manner that is consistent with the release branch maintenance policy

[brettlangdon]

It feels like this API needs to be more similar to our context provider API and just take Union[Context, Span], and remove the non_active_span property all together. You should do one or the other. If you are injecting just a context, then we don't need to grab the root and trigger a sampling decision (maybe?)

I keep going back and forth on whether log.error is right here. If there is an issue in one of our integrations we shouldn't error log, since the user cannot really do anything about it. But, if it is from manual instrumentation/injection the customer is doing, then the error log makes the most sense.

A compromise could be to use ddtrace.config._raise and either raise an exception or log. That way we can be sure at least none of our tests exhibit this behavior, and less likely to be something that happens in the wild with customers.

This is all just musings and talking out loud, I don't think I am asking for any specific changes to this PR.

[brettlangdon]

Suggested change

	"non_active_span in order to be sampled. span_context: {}, non_active_span.context: {}",
	"non_active_span in order to be sampled. span_context: %s, non_active_span.context: %s",

[brettlangdon]

Seems this line gets hit for a handful of our integrations, not sure I was expecting that.

[mabdinur]

I opened a PR to address this issue: #14206.

We should deprecate non_active_span