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

Make inclusion of railtie opt-in #156

Closed
wants to merge 1 commit into from

Conversation

ejholmes
Copy link
Contributor

@ejholmes ejholmes commented Jul 15, 2017

Closes #149

For those of us that want to use the dd-trace-rb gem without automatically including rails magic, this makes it easier. If people want the railtie, they can require 'ddtrace/rails' to get it.

This wouldn't be backwards compatible with existing installations, so maybe it would need to go in a 0.8 or 1.0 release.

@ufoot
Copy link
Contributor

ufoot commented Jul 18, 2017

Interesting. I'm indeed slightly worried about introducing a backward incompatible change. Currently trying to figure out where we could explicitly disable any op based on app config (but AFAIR there's a chicken and egg problem as when you have the app config it's sometimes too late). Another simple option is to opt-out based on some env var. Note that in non-Rails context, require ddtrace just does nothing (as Rails::VERSION is not here). I think the case "I use Rails and use ddtrace but don't want ddtrace to connect to Rails" is not mainstream, so would advocate for an opt-out. But it's true Rails is really an exception, as almost all other components need to be explicitly monkey patched.

@ejholmes
Copy link
Contributor Author

I hear you. An opt-out like how pry-rails does it would be alright, but would definitely prefer an explicit require 'ddtrace/rails' or a ddtrace-rails gem for the rails magic.

@ufoot
Copy link
Contributor

ufoot commented Aug 9, 2017

#173 implements the opt-out strategy

@ufoot ufoot closed this Aug 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants