Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ def initialize(config)
# @param payload [Hash] the payload passed as a method argument
# @return [Hash] the payload passed as a method argument
def start(_name, _id, payload)
span = OpenTelemetry::Instrumentation::Rack.current_span
return unless span.recording?

request = payload[:request]
# It seems that there are cases in Rails functional tests where it bypasses the routing system and the `action_dispatch.route_uri_pattern` header not being set.
# Our Test suite executes the routing system so we are unable to recreate this error case.
Expand All @@ -36,22 +39,18 @@ def start(_name, _id, payload)
attributes[OpenTelemetry::SemanticConventions::Trace::HTTP_ROUTE] = http_route if http_route
attributes[OpenTelemetry::SemanticConventions::Trace::HTTP_TARGET] = request.filtered_path if request.filtered_path != request.fullpath

span = OpenTelemetry::Instrumentation::Rack.current_span
span_name = if @span_naming == :semconv
if http_route
if @span_naming == :semconv
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we skip this entire logic unless span.recording??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a good improvement to make. Pushed up that change.

span.name = if http_route
"#{request.method} #{http_route}"
else
"#{request.method} /#{payload.dig(:params, :controller)}/#{payload.dig(:params, :action)}"
end
# If there is an exception we want to keep the original span name
# so it is easier to see where the request was routed to.
elsif request.env['action_dispatch.exception']
span.name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

API spans have a setter, but not a getter for name.

else
"#{payload[:controller]}##{payload[:action]}"
end
# If there is an exception we want to keep the original span name
# so it is easier to see where the request was routed to.
elsif !request.env['action_dispatch.exception']
span.name = "#{payload[:controller]}##{payload[:action]}"
end

span.name = span_name
span.add_attributes(attributes)
rescue StandardError => e
OpenTelemetry.handle_error(exception: e)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,12 @@
_(span.attributes['code.namespace']).must_equal 'ExceptionsController'
_(span.attributes['code.function']).must_equal 'show'
end

it 'does not raise with api/non recording spans' do
with_sampler(OpenTelemetry::SDK::Trace::Samplers::ALWAYS_OFF) do
get 'internal_server_error'
end
end
end

it 'sets filters `http.target`' do
Expand Down
8 changes: 8 additions & 0 deletions instrumentation/action_pack/test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,11 @@
c.use 'OpenTelemetry::Instrumentation::ActionPack'
c.add_span_processor span_processor
end

def with_sampler(sampler)
previous_sampler = OpenTelemetry.tracer_provider.sampler
OpenTelemetry.tracer_provider.sampler = sampler
yield
ensure
OpenTelemetry.tracer_provider.sampler = previous_sampler
end
Loading