fix(instr-http): respect requireParent flag when INVALID_SPAN_CONTEXT is used

[reberhardt7]

Which problem is this PR solving?

The http instrumentation (and many other instrumentation libraries) create spans with INVALID_SPAN_CONTEXT to create NonRecordingSpans when we enable requireParent and no parent is present: https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/http.ts#L769

That works fine; those spans are not exported. However, when we create child spans, those end up becoming real spans that are exported. E.g. if we have an http server, when a request comes in, we create a span with INVALID_SPAN_CONTEXT, and then if the http request handler makes an outbound request, the instrumentation sees that it has a parent span (even though it is invalid) and creates a real child span, initiating a new trace.

The opentelemetry sdk creates new traces for descendants of INVALID_SPAN_CONTEXT:

opentelemetry-js/packages/opentelemetry-sdk-trace-base/src/Tracer.ts

Line 91 in ecc88a3

!api.trace.isSpanContextValid(parentSpanContext)

Short description of the changes

This checks whether the parent span is recording, and does not create a real child span if the parent isn't recording.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Added unit test to verify fix

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: reberhardt7 / name: Ryan Eberhardt (658e198)

[reberhardt7]

I've signed the CLA, but it looks like the CLA bot hasn't updated yet. I'm hoping it'll update automatically in a few hours or something like that

[david-luna]

ping to other reviewers @pichlermarc @JamieDanielson @dyladan @trentm

[trentm]

I think it is correct that no span (trace.getSpan(ctx) === undefined) and an invalid span (api.trace.isSpanContextValid(trace.getSpan(ctx)) === false) should be treated as equivalent for requireParent config vars.

However, I'm not super-confident, so I was hoping to get review from some of the longer term reviewers.

Should suppressTracing, isTracingSuppressed (APIs from @opentelemetry/core) possibly fit in here for the presumed use case?

I can understand the desire to use requireParentforIncomingSpans=true to only get traces for "interesting" incoming requests to a service. (I'm guessing at the use case.)
This means that every instrumentation that can generate outgoing spans would need to have a requireParent config and the user would have it set it true for all of them, right?

[reberhardt7]

Yeah, my use case is incrementally adding tracing to an existing service that gets a lot of traffic, and I want to be able to gradually increase the coverage of traces to decrease the risk of accidentally degrading prod performance from over-instrumentation. It has been a while since I made this change, so I forgot exactly what was happening, but I think we had requireParentforIncomingSpans=true in a few services that we only wanted to trace in the context of requests from another service.

I'm not sure whether isTracingSuppressed would fit in here. My guess is no, because we don't necessarily want to actively suppress any child spans that are created from this context, in case we legitimately want to start tracing later on.

[reberhardt7]

@pichlermarc @JamieDanielson @dyladan @trentm could I get a review when you have time? Much appreciated :)

[reberhardt7]

@pichlermarc @JamieDanielson @dyladan @trentm would love to land this PR if possible

[JamieDanielson]

I think it is correct that no span (trace.getSpan(ctx) === undefined) and an invalid span (api.trace.isSpanContextValid(trace.getSpan(ctx)) === false) should be treated as equivalent for requireParent config vars.

I agree with this. I think what we're still trying to sort out is how we get into a state of an invalid span context here.

@reberhardt7 Is there an issue with the client / is the client not behaving according to spec? Or is this somehow being set intentionally while rolling out the incremental instrumentation?