Migrate sinatra to new configuration API
#226
Merged
Conversation
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
palazzem
added
core
palazzem
changed the base branch from
pedro/create-configurable-module
to
master
palazzem
suggested changes
Oct 30, 2017
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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!
palazzem
suggested changes
Nov 15, 2017
| @@ -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 | |||
There was a problem hiding this comment.
@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!
p-lambert
force-pushed
the
pedro/migrate-sinatra-configuration
branch
from
4ab41d2
to
ea3153e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Overview
This PR migrates the
sinatraintegration to the new configuration API.Usage Example