feat: suppress internal spans with Faraday instrumentation

[ezhang6811]

Addresses #1502 for Faraday library instrumentation. Allow user to set suppress_internal_instrumentation flag when configuring Faraday instrumentation to suppress additional client spans generated from underlying Net::Http instrumentation.

Before merging, waiting for update on Github issue to see if internal span suppression can be made default behavior for both Faraday and AWS SDK.

[arielvalentin]

I believe that using untraced will suppress instrumentation for all child spans.

Is this what you want?

[ezhang6811]

Yes, currently Faraday will generate child spans for its under-the-hood Net::Http instrumentation which creates an additional dependency metric for each request. We want to give the customer the option (preferably default) to suppress this.

[arielvalentin]

I see now, I misunderstood your goals here.

You only want to retain Faraday client spans and skip any driver generated spans.

Is that correct?

I think that may be an invalid combination here where if the Faraday span is configured to generate internal spans only and also suppress driver spans.

I wonder then if what we want here is separate middleware implementations as opposed to conditional checks I.e. replacing conditional logic with polymorphism.

That would likely couple the span kind setting to this suppression setting.

[ezhang6811]

Are you referring to the configuration below?

'OpenTelemetry::Instrumentation::Faraday' => {
    suppress_internal_instrumentation: true,
    span_kind: :internal
}

This may not make sense, but I think flexibility to configure the options separately is better. Some users may also want client spans where the internal instrumentation isn't suppressed for better transparency, which discourages the coupling of the two settings.

Also, why do you prefer the polymorphism approach when the conditional is simple enough?

[arielvalentin]

Are you referring to the configuration below?

'OpenTelemetry::Instrumentation::Faraday' => {

    suppress_internal_instrumentation: true,

    span_kind: :internal

}

This may not make sense, but I think flexibility to configure the options separately is better. Some users may also want client spans where the internal instrumentation isn't suppressed for better transparency, which discourages the coupling of the two settings.

The more configuration options we offer the more complex the instrumentations are to understand for our users.

The combination of having an internal span and also suppressing the child span would be an invalid combination. I don't think it's one we want to support.

Also, why do you prefer the polymorphism approach when the conditional is simple enough?

Yes, I agree that this conditional check is a minimal implementation but I also believe that the instrumentation should do the minimal possible to achieve its goal. The config option cannot change for the life of the instrumentation so we are checking it with each request.

Yes, it will likely be optimized away by a JIT at some point and it's just a quick check, but for now anything we can remove from the users request path we should consider doing. Keep in mind that my comment was more of a nitpick and not a blocker.

[arielvalentin]

What about adding an option here like :none?

[ezhang6811]

Could you elaborate on this? The current behavior of setting the flag to false will ensure no spans are suppressed.