Skip to content

Commit

Permalink
fix!: remove enable_recognize_route and span_naming options (#214)
Browse files Browse the repository at this point in the history 
`recognize_route` can end up mutating the request object in the event it
processes a potential match that is not anchored. That is, shall we say,
not ideal. (And causes production applications to break).

I was initially going to just default the options to `off`
(essentially), but why leave potentially dangerous code lying about?
Also, it ended up being easier to just yank stuff out rather than fix
the test cases to handle the inverse of what the expected, etc. So, this
commit just removes it all.

Step backwards in convenience, sure. Big step forward in safety? Definitely.

For context: https://github.com/open-telemetry/opentelemetry-ruby-contrib/pull/123/files#r1035711863
  • Loading branch information
@ahayworth
ahayworth committed Dec 2, 2022
1 parent 207369a commit ea604aa
Show file tree
Hide file tree
Showing 5 changed files with 7 additions and 148 deletions.
18 changes: 0 additions & 18 deletions instrumentation/action_pack/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,24 +30,6 @@ OpenTelemetry::SDK.configure do |c|
end
```

### Configuration options

| Name | Default | Description |
| ----- | ------- | ----------- |
| `span_naming` | `:rails_route` | Configures the name for the Rack span. `:rails_route` is in the format of `HTTP_METHOD /rails/route(.:format)`, for example `GET /users/:id(.:format)`. `:controller_action` is in the format of `Controller#action`, for example `UsersController#show` |
| `enable_recognize_route` | `true` | Enables or disables adding the `http.route` attribute. |

The default configuration uses a [method from Rails to obtain the route for the request](https://github.com/rails/rails/blob/v6.1.3/actionpack/lib/action_dispatch/journey/router.rb#L65). The options above allow this behaviour to be opted out of if you have performance issues. If you wish to avoid using this method then set `span_naming: :controller_action, enable_recognize_route: false`.

```ruby
OpenTelemetry::SDK.configure do |c|
c.use 'OpenTelemetry::Instrumentation::ActionPack', {
span_naming: :controller_action,
enable_recognize_route: false
}
end
```

## Examples

Example usage can be seen in the `./example/trace_demonstration.rb` file [here](https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/action_pack/example/trace_demonstration.ru)
Expand Down
3 changes: 0 additions & 3 deletions instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/instrumentation.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base
gem_version >= MINIMUM_VERSION
end

option :enable_recognize_route, default: true, validate: :boolean
option :span_naming, default: :rails_route, validate: %i[controller_action rails_route]

private

def gem_version
Expand Down
15 changes: 1 addition & 14 deletions ...ion_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,12 @@ module Metal
def dispatch(name, request, response)
rack_span = OpenTelemetry::Instrumentation::Rack.current_span
if rack_span.recording?
unless request.env['action_dispatch.exception']
rack_span.name = case instrumentation_config[:span_naming]
when :controller_action then "#{self.class.name}##{name}"
else "#{request.method} #{rails_route(request)}"
end
end
rack_span.name = "#{self.class.name}##{name}" unless request.env['action_dispatch.exception']

attributes_to_append = {
OpenTelemetry::SemanticConventions::Trace::CODE_NAMESPACE => self.class.name,
OpenTelemetry::SemanticConventions::Trace::CODE_FUNCTION => name
}
attributes_to_append[OpenTelemetry::SemanticConventions::Trace::HTTP_ROUTE] = rails_route(request) if instrumentation_config[:enable_recognize_route]
attributes_to_append[OpenTelemetry::SemanticConventions::Trace::HTTP_TARGET] = request.filtered_path if request.filtered_path != request.fullpath
rack_span.add_attributes(attributes_to_append)
end
Expand All @@ -35,13 +29,6 @@ def dispatch(name, request, response)

private

def rails_route(request)
@rails_route ||= ::Rails.application.routes.router.recognize(request) do |route, _params|
return route.path.spec.to_s
# Rails will match on the first route - see https://guides.rubyonrails.org/routing.html#crud-verbs-and-actions
end
end

def instrumentation_config
ActionPack::Instrumentation.instance.config
end
Expand Down
116 changes: 5 additions & 111 deletions ...ck/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@
# Clear captured spans
before { exporter.reset }

it 'sets the span name to the format: HTTP_METHOD /rails/route(.:format)' do
it 'sets the span name to the format: ControllerName#action' do
get '/ok'

_(last_response.body).must_equal 'actually ok'
_(last_response.ok?).must_equal true
_(span.name).must_equal 'GET /ok(.:format)'
_(span.name).must_equal 'ExampleController#ok'
_(span.kind).must_equal :server
_(span.status.ok?).must_equal true

Expand All @@ -38,7 +38,6 @@
_(span.attributes['http.target']).must_equal '/ok'
_(span.attributes['http.status_code']).must_equal 200
_(span.attributes['http.user_agent']).must_be_nil
_(span.attributes['http.route']).must_equal '/ok(.:format)'
_(span.attributes['code.namespace']).must_equal 'ExampleController'
_(span.attributes['code.function']).must_equal 'ok'
end
Expand All @@ -49,7 +48,7 @@

_(last_response.body).must_equal 'created new item'
_(last_response.ok?).must_equal true
_(span.name).must_equal 'GET /items/new(.:format)'
_(span.name).must_equal 'ExampleController#new_item'
_(span.kind).must_equal :server
_(span.status.ok?).must_equal true

Expand All @@ -62,15 +61,14 @@
_(span.attributes['http.target']).must_equal '/items/new'
_(span.attributes['http.status_code']).must_equal 200
_(span.attributes['http.user_agent']).must_be_nil
_(span.attributes['http.route']).must_equal '/items/new(.:format)'
_(span.attributes['code.namespace']).must_equal 'ExampleController'
_(span.attributes['code.function']).must_equal 'new_item'
end

it 'sets the span name when the controller raises an exception' do
get 'internal_server_error'

_(span.name).must_equal 'GET /internal_server_error(.:format)'
_(span.name).must_equal 'ExampleController#internal_server_error'
end

it 'does not set the span name when an exception is raised in middleware' do
Expand All @@ -85,117 +83,13 @@
_(span.name).must_equal 'HTTP GET'
end

describe 'when the application has span_naming set with controller_action' do
before do
OpenTelemetry::Instrumentation::ActionPack::Instrumentation.instance.config[:span_naming] = :controller_action
end

after do
OpenTelemetry::Instrumentation::ActionPack::Instrumentation.instance.config[:span_naming] = nil
end

it 'sets the span name to the HTTP method and route' do
get '/ok'

_(span.name).must_equal 'ExampleController#ok'
end

it 'sets the http.route attribute' do
get '/ok'

_(span.attributes['http.route']).must_equal '/ok(.:format)'
end

it 'sets the span name when the controller raises an exception' do
get 'internal_server_error'

_(span.name).must_equal 'ExampleController#internal_server_error'
end

it 'does not set the span name when an exception is raised in middleware' do
get '/ok?raise_in_middleware'

_(span.name).must_equal 'HTTP GET'
end

it 'does not set the span name when the request is redirected in middleware' do
get '/ok?redirect_in_middleware'

_(span.name).must_equal 'HTTP GET'
end
end

describe 'when it is installed by OpenTelemetry::SDK' do
let(:config) { { span_naming: :controller_action, enable_recognize_route: false } }
let(:default_config) { { span_naming: :rails_route, enable_recognize_route: true } }

before(:each) do
# Clear the current instance, so we can call OpenTelemetry::SDK.configure to install a new instance
OpenTelemetry::Instrumentation::ActionPack::Instrumentation.instance_variable_set('@instance', nil)
end
after(:each) do
# Restore the default instance
OpenTelemetry::Instrumentation::ActionPack::Instrumentation.instance_variable_set('@instance', nil)
OpenTelemetry::SDK.configure do |c|
c.use 'OpenTelemetry::Instrumentation::ActionPack'
end
end

it 'sets span_naming and enable_recognize_route' do
OpenTelemetry::SDK.configure do |c|
c.use 'OpenTelemetry::Instrumentation::ActionPack', config
end

_(OpenTelemetry::Instrumentation::ActionPack::Instrumentation.instance.config).must_equal config
end

it 'uses default values for span_naming and enable_recognize_route' do
OpenTelemetry::SDK.configure do |c|
c.use 'OpenTelemetry::Instrumentation::ActionPack'
end
_(OpenTelemetry::Instrumentation::ActionPack::Instrumentation.instance.config).must_equal default_config
end
end

describe 'when the application has exceptions_app configured' do
let(:rails_app) { AppConfig.initialize_app(use_exceptions_app: true) }

it 'does not overwrite the span name from the controller that raised' do
get 'internal_server_error'

_(span.name).must_equal 'GET /internal_server_error(.:format)'
end
end

describe 'when the application has enable_recognize_route disabled' do
before do
OpenTelemetry::Instrumentation::ActionPack::Instrumentation.instance.config[:enable_recognize_route] = false
end

after do
OpenTelemetry::Instrumentation::ActionPack::Instrumentation.instance.config[:enable_recognize_route] = true
end

it 'sets uses the :rails_route span naming' do
get '/items/new'
_(last_response.body).must_equal 'created new item'
_(last_response.ok?).must_equal true
_(span.name).must_equal 'GET /items/new(.:format)'
_(span.kind).must_equal :server
_(span.status.ok?).must_equal true

_(span.instrumentation_library.name).must_equal 'OpenTelemetry::Instrumentation::Rack'
_(span.instrumentation_library.version).must_equal OpenTelemetry::Instrumentation::Rack::VERSION

_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.host']).must_equal 'example.org'
_(span.attributes['http.scheme']).must_equal 'http'
_(span.attributes['http.target']).must_equal '/items/new'
_(span.attributes['http.status_code']).must_equal 200
_(span.attributes['http.user_agent']).must_be_nil
_(span.attributes['http.route']).must_be_nil
_(span.attributes['code.namespace']).must_equal 'ExampleController'
_(span.attributes['code.function']).must_equal 'new_item'
_(span.name).must_equal 'ExampleController#internal_server_error'
end
end

Expand Down
3 changes: 1 addition & 2 deletions ...strumentation/opentelemetry/instrumentation/rails/patches/action_controller/metal_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

_(last_response.body).must_equal 'actually ok'
_(last_response.ok?).must_equal true
_(span.name).must_equal 'GET /ok(.:format)'
_(span.name).must_equal 'ExampleController#ok'
_(span.kind).must_equal :server
_(span.status.ok?).must_equal true

Expand All @@ -35,7 +35,6 @@
_(span.attributes['http.target']).must_equal '/ok'
_(span.attributes['http.status_code']).must_equal 200
_(span.attributes['http.user_agent']).must_be_nil
_(span.attributes['http.route']).must_equal '/ok(.:format)'
_(span.attributes['code.namespace']).must_equal 'ExampleController'
_(span.attributes['code.function']).must_equal 'ok'
end
Expand Down

0 comments on commit ea604aa

Please sign in to comment.