Always check if valid span returned from HttpClientTracer.startSpan

[trask]

We need to check everywhere if the span returned from TRACER.startSpan() is invalid before starting a scope with it.

Tracking issue for #893 (review)

Relates to #465

[anuraaga]

Should we automatically return a no-op scope when starting one with invalid span?

[anuraaga]

Here's reference from brave - https://github.com/openzipkin/brave/blob/ae2b26adda5ffd6080f698024a20e81c32c291fe/brave/src/main/java/brave/Tracer.java#L599

[trask]

#893 revealed a few places that complicate things, in particular where we can't(?) do context injection inside of our TRACER.startSpan() (a couple examples below)

i sort of like the idea of having a consistent template (e.g. check isValid everywhere) vs checking isValid in some cases but not in others.

but i'm also concerned about the growing size of our "consistent template"

      callDepth = TRACER.getCallDepth();
      if (callDepth.getAndIncrement() == 0) {
        span = TRACER.startSpan(request);

        Context context = withSpan(span, Context.current());
        if (request != null) {
          AkkaHttpHeaders headers = new AkkaHttpHeaders(request);
          OpenTelemetry.getPropagators().getHttpTextFormat().inject(context, request, headers);
          // Request is immutable, so we have to assign new value once we update headers
          request = headers.getRequest();
        }
        scope = withScopedContext(context);
      }

    Span span = TRACER.startSpan(chain.request());
    Context context = withSpan(span, Context.current());
    Request.Builder requestBuilder = chain.request().newBuilder();
    OpenTelemetry.getPropagators()
        .getHttpTextFormat()
        .inject(context, requestBuilder, RequestBuilderInjectAdapter.SETTER);

    Response response;
    try (Scope scope = withScopedContext(context)) {
      response = chain.proceed(requestBuilder.build());
    } catch (final Exception e) {
      TRACER.endExceptionally(span, e);
      throw e;
    }
    TRACER.end(span, response);
    return response;

[anuraaga]

Since injection should also be no-op for an invalid span, it looks like these work OK even if the span is invalid. There's a bit of overhead though due to the execution of the no-op methods but I guess it keeps things simple?

[trask]

this is a really good point 👍

[trask]

Closing, agree with @anuraaga's point above, and we can optimize as needed (in particular see #1009)