Skip to content

feat: suppress internal spans with Faraday instrumentation #1506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+14 −1
Open

feat: suppress internal spans with Faraday instrumentation #1506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be02830
add flag to suppress downstream instrumentation
ezhang6811 Apr 26, 2025
7d01f05
update README
ezhang6811 Apr 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions instrumentation/faraday/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ To install the instrumentation, call `use` with the name of the instrumentation.
```ruby
OpenTelemetry::SDK.configure do |c|
c.use 'OpenTelemetry::Instrumentation::Faraday'
suppress_internal_instrumentation: true
end
```

Expand All @@ -30,6 +31,11 @@ OpenTelemetry::SDK.configure do |c|
end
```

### Configuration options
This instrumentation offers the following configuration options:
* `suppress_internal_instrumentation` (default: `false`): When set to `true`, any spans with
span kind of `internal` are suppressed from traces.

## Examples

Example usage of faraday can be seen in the `./example/faraday.rb` file [here](https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/faraday/example/faraday.rb)
Expand Down
1 change: 1 addition & 0 deletions instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/instrumentation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base

option :span_kind, default: :client, validate: %i[client internal]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding an option here like :none?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

option :peer_service, default: nil, validate: :string
option :suppress_internal_instrumentation, default: false, validate: :boolean

private

Expand Down
8 changes: 7 additions & 1 deletion ...tation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,13 @@ def call(env)
) do |span|
OpenTelemetry.propagation.inject(env.request_headers)

app.call(env).on_complete { |resp| trace_response(span, resp.status) }
if config[:suppress_internal_instrumentation]
OpenTelemetry::Common::Utilities.untraced do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Is this what you want?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

app.call(env).on_complete { |resp| trace_response(span, resp.status) }
end
else
app.call(env).on_complete { |resp| trace_response(span, resp.status) }
end
rescue ::Faraday::Error => e
trace_response(span, e.response[:status]) if e.response

Expand Down
Loading