Skip to content
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

Jump to bottom

fix!: remove enable_recognize_route and span_naming options #214

Merged
merged 1 commit into from
Dec 2, 2022
+7 −148
Merged

fix!: remove enable_recognize_route and span_naming options #214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 0 additions & 18 deletions instrumentation/action_pack/README.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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']
Copy link
Contributor

Choose a reason for hiding this comment

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

In days of yore, it looks like we checked for valid context on the rack span. Not sure if we want to bring that back; not a blocker, just a detail I wanted to surface.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@plantfansam I may be incorrect here but I think that checking if it is read/write span i.e. recording?, is better than checking for a valid context.

If I understand this code correctly, it is possible to have a read-only span that has a valid context:

https://github.com/open-telemetry/opentelemetry-ruby/blob/a4efaf3a44b0efc6ca805a6d0a5bdc4f13a01b34/sdk/lib/opentelemetry/sdk/trace/tracer_provider.rb#LL161C81-L161C81

I think that means it would call the no-opt name and add_attributes methods: https://github.com/open-telemetry/opentelemetry-ruby/blob/a4efaf3a44b0efc6ca805a6d0a5bdc4f13a01b34/api/lib/opentelemetry/trace/span.rb#L138

Copy link
Contributor

@plantfansam plantfansam Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I read the code the same way @arielvalentin, and agree with your reasoning!


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
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

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

These basic scenarios are basically copy/pasta from the above, which is why it's ok to remove.

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
Open in desktop
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