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

Migrate sinatra to new configuration API #226

Merged
merged 1 commit into from
Nov 21, 2017
Merged

Migrate sinatra to new configuration API #226

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
2 changes: 1 addition & 1 deletion lib/ddtrace/contrib/active_record/patcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def patch_active_record
def self.datadog_trace
# TODO: Consider using patcher for Rails as well.
# @tracer ||= defined?(::Rails) && ::Rails.configuration.datadog_trace
@datadog_trace ||= defined?(::Sinatra) && ::Sinatra::Application.settings.datadog_tracer.cfg
@datadog_trace ||= defined?(::Sinatra) && Datadog.configuration[:sinatra].to_h
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be interesting if we can avoid this breaking change. Maybe we can use the legacy instrumentation with priority if it's available, falling back to the new one. Consider that the real use case is:

  • I have the legacy instrumentation
  • I replace it with the new one

It's legit to say "having the legacy instrumentation AND the new one doesn't work fine". Do we have other blockers?

Copy link
Contributor

Choose a reason for hiding this comment

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

@p-lambert ok we can make this breaking change, but it would be great testing in a real application other than relying on tests. Thanks a lot!

end

def self.adapter_name
Expand Down
86 changes: 23 additions & 63 deletions lib/ddtrace/contrib/sinatra/tracer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,63 +17,33 @@
module Datadog
module Contrib
module Sinatra
# TracerCfg is used to manipulate the configuration of the Sinatra
# tracing extension.
class TracerCfg
DEFAULT_CFG = {
enabled: true,
default_service: 'sinatra',
tracer: Datadog.tracer,
debug: false,
trace_agent_hostname: Datadog::Writer::HOSTNAME,
trace_agent_port: Datadog::Writer::PORT
}.freeze()

attr_accessor :cfg

def initialize
@cfg = DEFAULT_CFG.dup()
end

def configure(args = {})
args.each do |name, value|
self[name] = value
end
# Datadog::Contrib::Sinatra::Tracer is a Sinatra extension which traces
# requests.
module Tracer
include Base
register_as :sinatra

apply()
option :enabled, default: true, depends_on: [:tracer] do |value|
get_option(:tracer).enabled = value
end

def apply
Datadog::Tracer.debug_logging = @cfg[:debug]

tracer = @cfg[:tracer]

tracer.enabled = @cfg[:enabled]
tracer.configure(hostname: @cfg[:trace_agent_hostname],
port: @cfg[:trace_agent_port])

tracer.set_service_info(@cfg[:default_service], 'sinatra',
Datadog::Ext::AppTypes::WEB)
option :default_service, default: 'sinatra', depends_on: [:tracer] do |value|
get_option(:tracer).set_service_info(value, 'sinatra', Ext::AppTypes::WEB)
value
end

def [](key)
raise ArgumentError, "unknown setting '#{key}'" unless @cfg.key? key
@cfg[key]
end
option :tracer, default: Datadog.tracer

def []=(key, value)
raise ArgumentError, "unknown setting '#{key}'" unless @cfg.key? key
@cfg[key] = value
option(:debug, default: false) { |value| Tracer.debug_logging = value }

option :trace_agent_hostname, default: Writer::HOSTNAME, depends_on: [:tracer] do |value|
get_option(:tracer).configure(hostname: value)
end

def enabled?
@cfg[:enabled] && !@cfg[:tracer].nil?
option :trace_agent_port, default: Writer::PORT, depends_on: [:tracer] do |value|
get_option(:tracer).configure(port: value)
end
end

# Datadog::Contrib::Sinatra::Tracer is a Sinatra extension which traces
# requests.
module Tracer
def route(verb, action, *)
# Keep track of the route name when the app is instantiated for an
# incoming request.
Expand All @@ -89,11 +59,9 @@ def route(verb, action, *)
def self.registered(app)
::Sinatra::Base.module_eval do
def render(engine, data, *)
cfg = settings.datadog_tracer

output = ''
if cfg.enabled?
tracer = cfg[:tracer]
tracer = Datadog.configuration[:sinatra][:tracer]
if tracer.enabled
tracer.trace('sinatra.render_template') do |span|
# If data is a string, it is a literal template and we don't
# want to record it.
Expand All @@ -108,15 +76,8 @@ def render(engine, data, *)
end
end

app.set :datadog_tracer, TracerCfg.new()

app.configure do
app.settings.datadog_tracer.apply()
end

app.before do
cfg = settings.datadog_tracer
return unless cfg.enabled?
return unless Datadog.configuration[:sinatra][:tracer].enabled

if instance_variable_defined? :@datadog_request_span
if @datadog_request_span
Expand All @@ -126,10 +87,10 @@ def render(engine, data, *)
end
end

tracer = cfg[:tracer]
tracer = Datadog.configuration[:sinatra][:tracer]

span = tracer.trace('sinatra.request',
service: cfg.cfg[:default_service],
service: Datadog.configuration[:sinatra][:default_service],
span_type: Datadog::Ext::HTTP::TYPE)
span.set_tag(Datadog::Ext::HTTP::URL, request.path)
span.set_tag(Datadog::Ext::HTTP::METHOD, request.request_method)
Expand All @@ -138,8 +99,7 @@ def render(engine, data, *)
end

app.after do
cfg = settings.datadog_tracer
return unless cfg.enabled?
return unless Datadog.configuration[:sinatra][:tracer].enabled

span = @datadog_request_span
begin
Expand Down
4 changes: 2 additions & 2 deletions test/contrib/sinatra/tracer_activerecord_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ def setup
app().set :datadog_test_writer, @writer

tracer = Datadog::Tracer.new(writer: @writer)
app().settings.datadog_tracer.configure(tracer: tracer, enabled: true)
Datadog.configuration.use(:sinatra, tracer: tracer, enabled: true)

conn = ActiveRecord::Base.establish_connection(adapter: 'sqlite3',
database: ':memory:')
app().set :datadog_test_conn, conn

Datadog::Monkey.patch_module(:active_record)
Datadog.configuration.use(:active_record)

super
end
Expand Down
2 changes: 1 addition & 1 deletion test/contrib/sinatra/tracer_disabled_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def setup
app().set :datadog_test_writer, @writer

tracer = Datadog::Tracer.new(writer: @writer)
app().settings.datadog_tracer.configure(tracer: tracer, enabled: false)
Datadog.configuration.use(:sinatra, tracer: tracer, enabled: false)

super
end
Expand Down
2 changes: 1 addition & 1 deletion test/contrib/sinatra/tracer_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def setup
app().set :datadog_test_writer, @writer

tracer = Datadog::Tracer.new(writer: @writer)
app().settings.datadog_tracer.configure(tracer: tracer, enabled: true)
Datadog.configuration.use(:sinatra, tracer: tracer, enabled: true)

super
end
Expand Down