Update instrumentors to use span processors

[ocelotl]

Fixes #832

This can be implemented as a fix right now, but the issue highlights a more pressing matter, the fact that there is an overlap between configuration in the standard sense (the kind that uses Configuration) and the kind that is implemented by the end user as application code.

[lzchen]

Can you explain a bit more about what the issue is and how we are solving it? Is this a quick fix or a permanent solution? I also am confused with this: the fact that there is an overlap between configuration in the standard sense (the kind that uses Configuration) and the kind that is implemented by the end user as application code, how does this relate to the problem? An example would be nice.

[ocelotl]

Can you explain a bit more about what the issue is and how we are solving it? Is this a quick fix or a permanent solution? I also am confused with this: the fact that there is an overlap between configuration in the standard sense (the kind that uses Configuration) and the kind that is implemented by the end user as application code, how does this relate to the problem? An example would be nice.

Sure, the problem here happens with auto instrumentation. The example from @mat-rumian in #832 used auto instrumentation to launch several scripts. Among them, this code was getting executed:

from opentelemetry import trace
from opentelemetry.ext.jaeger import JaegerSpanExporter
from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.sdk.trace.export import (
    ConsoleSpanExporter,
    SimpleExportSpanProcessor,
    BatchExportSpanProcessor,
)
import opentelemetry.ext.requests


def configure_jaeger_span_exporter(configuration: dict):
    exporter = JaegerSpanExporter(
        service_name=configuration['service_name'],
        agent_host_name=configuration['agent_host'],
        agent_port=int(configuration['agent_port']),
    )

    trace.set_tracer_provider(TracerProvider())
    trace.get_tracer(__name__)

    span_processor = BatchExportSpanProcessor(exporter)
    trace.get_tracer_provider().add_span_processor(span_processor)

    trace.get_tracer_provider().add_span_processor(
        SimpleExportSpanProcessor(ConsoleSpanExporter())
    )

Span processors were added to the tracer, but this happened after the auto instrumentation process happened. In this process, a tracer is created (without span processors). This object is stored in a function as a closure: here and here.

So, the requests and the dbapi instrumentations end up using this tracer object that lacks the span processors and that makes their spans not to be exported and that is why they were not showing up in the Jaeger exporter.

This is still a draft, it could be added as a fix in the instrumentations, it will solve the issue in these two instrumentations but I think there is a bigger issue, the fact that some configuration can happen in the environment variables (configuration that is accessible to the auto instrumentation process) and some can happen in the application code (configuration that is not accessible to the auto instrumentation process, like the addition of span processors in the application code).

[toumorokoshi]

Yeah, this is a behavioral issue that has bitten me a little bit to. You have to be very careful with the ordering of the creation of the required components:

1. named tracer, which binds to whatever trace provider (noop by default)
2. instrumentation, which can be designed to create a name tracer on instrumentation right away, or deferred
3. trace provider.

To make this fool-proof, we need to ensure creation of the TracerProvider before auto-instrumentation. This is hard to do, because one typically configures their TraceProvider programatically, after the application initializes.

I agree with @ocelotl that the best option is to defer tracer creation until we absolutely have to. Not sure how we can codify it though, besides documenting that practice somewhere and hoping that others will follow it.

[toumorokoshi]

I think there's optimizations for processor creation, but there may be value in tackling that at a higher level.

This does resolve a critical issue with trace configuration with auto-instrumentation. Thanks for the fix!

[lzchen]

Nice