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.

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

[active_record] decouple ActiveRecord and Sinatra integrations #330

Merged
merged 3 commits into from
Jan 26, 2018

Conversation

palazzem
Copy link
Contributor

Overview

Closes #328.

When ActiveRecord is used standalone it should work without expecting other integrations to be activated. The following must be a legit instrumentation:

Datadog.configure do |config|
  config.use :active_record, service_name: 'random-service'
  config.use :grape, service_name: 'random-service'
end

@palazzem palazzem added the integrations Involves tracing integrations label Jan 26, 2018
@palazzem palazzem added this to the 0.11.1 milestone Jan 26, 2018
@palazzem palazzem requested a review from ufoot January 26, 2018 17:26
ufoot
ufoot previously approved these changes Jan 26, 2018
Copy link
Contributor

@ufoot ufoot left a comment

Choose a reason for hiding this comment

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

Let's wait for CI verdict because old Ruby versions can be full of surprises, but looks good, thanks !

@@ -57,22 +58,18 @@ def self.adapter_port
@adapter_port ||= Datadog::Contrib::Rails::Utils.adapter_port
end

def self.tracer
Copy link
Contributor

Choose a reason for hiding this comment

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

So normally as this is quite internal, nobody was using this. I let you decide wether it's worth mentioning in the release notes, but I'd say no.

end
end

it 'calls the instrumentation when is used standalone' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this could be considered a regression test, since undoing the patch doesn't make it pass

def self.database_service
return @database_service if defined?(@database_service)

@database_service = get_option(:service_name) || adapter_name
tracer.set_service_info(@database_service, 'sinatra', Ext::AppTypes::DB)
get_option(:tracer).set_service_info(@database_service, 'sinatra', Ext::AppTypes::DB)

Choose a reason for hiding this comment

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

Why does this still say 'sinatra'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hawknewton correct! removing it!

@palazzem palazzem force-pushed the palazzem/activerecord-sinatra-fix branch from 4e94e37 to 2be20c3 Compare January 26, 2018 20:19
expect(span.get_tag('sql.query')).to eq(nil)
end

it 'sends the right service information' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hawknewton added this integration test for the wrong service name you've discovered. Now it should be fine, thank you very much!

Choose a reason for hiding this comment

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

@palazzem Awesome, thank you very much.

@palazzem palazzem force-pushed the palazzem/activerecord-sinatra-fix branch from fc319c4 to 3816ea9 Compare January 26, 2018 22:31
@palazzem palazzem merged commit daaf5fe into master Jan 26, 2018
@palazzem palazzem deleted the palazzem/activerecord-sinatra-fix branch January 26, 2018 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants