-
Notifications
You must be signed in to change notification settings - Fork 156
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.
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
fix!: remove enable_recognize_route and span_naming options #214
Merged
arielvalentin
merged 1 commit into
open-telemetry:main
from
ahayworth:ahayworth/ditch-rails-route-stuff
Dec 2, 2022
+7
−148
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
|
@@ -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 | ||
|
@@ -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 | ||
|
||
|
@@ -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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
andadd_attributes
methods: https://github.com/open-telemetry/opentelemetry-ruby/blob/a4efaf3a44b0efc6ca805a6d0a5bdc4f13a01b34/api/lib/opentelemetry/trace/span.rb#L138There was a problem hiding this comment.
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!