changes to option proc loads broke default usage #3182
Comments
HoneyryderChuck
added
bug
|
Thank you so much for opening the issue. Yes, from version Here is the list of PR with all the changes: The whole idea was to simplify the configuration DSL. Looking at your configuration code, running on the latest ddtrace version your configuration could look like this: module Datadog
module Tracing
module Contrib
module Tobox
module Configuration
class Settings < Contrib::Configuration::Settings
option :enabled do |o|
o.env "DD_TOBOX_SIDEKIQ_ENABLED"
o.type :bool
o.default true
end
option :analytics_enabled do |o|
o.env "DD_TOBOX_ANALYTICS_ENABLED"
o.type :bool
o.default false
end
option :analytics_sample_rate do |o|
o.env "DD_TRACE_TOBOX_ANALYTICS_SAMPLE_RATE"
o.type :float
o.default 1.0
end
option :service_name
option :error_handler do |o|
o.type :proc
o.default_proc(&Tracing::SpanOperation::Events::DEFAULT_ON_ERROR)
end
option :distributed_tracing, default: false
end
end
end
end
end
endI see a problem in your case, as your plugin does not restrict the ddtrace version, meaning you must maintain multiple ddtrace versions. There is a workaround for this case's You could try doing: option :error_handler, default: -> { Tracing::SpanOperation::Events::DEFAULT_ON_ERROR } That would evaluate the proc, and it returns another proc. It is a workaround. The issue with your code is that methods like You could have a separate configuration DSL based on the ddtrace version. As you did in your patch, it is not ideal, but also not a huge burden when it comes to maintaining it as the configuration DSL should not change much in the future. require "datadog/tracing/span_operation"
module Datadog
module Tracing
module Contrib
module Tobox
module Configuration
class Settings < Contrib::Configuration::Settings
if DDTrace::VERSION::STRING >= "1.13.0"
option :enabled do |o|
o.env "DD_TOBOX_SIDEKIQ_ENABLED"
o.type :bool
o.default true
end
option :analytics_enabled do |o|
o.env "DD_TOBOX_ANALYTICS_ENABLED"
o.type :bool
o.default false
end
option :analytics_sample_rate do |o|
o.env "DD_TRACE_TOBOX_ANALYTICS_SAMPLE_RATE"
o.type :float
o.default 1.0
end
option :service_name
option :error_handler do |o|
o.type :proc
# Workaround to avoid having to use `experimental_default_prc` or `default_proc`
o.default -> { Tracing::SpanOperation::Events::DEFAULT_ON_ERROR }
end
option :distributed_tracing, default: false
elsif
option :enabled do |o|
o.default { env_to_bool("DD_TOBOX_SIDEKIQ_ENABLED", true) }
o.lazy
end
option :analytics_enabled do |o|
o.default { env_to_bool("DD_TOBOX_ANALYTICS_ENABLED", false) }
o.lazy
end
option :analytics_sample_rate do |o|
o.default { env_to_float("DD_TRACE_TOBOX_ANALYTICS_SAMPLE_RATE", 1.0) }
o.lazy
end
option :service_name
option :error_handler, default: Tracing::SpanOperation::Events::DEFAULT_ON_ERROR
option :distributed_tracing, default: false
end
end
end
end
end
end
endAnother option would be to specify the minimum version of ddtrace on your project, and once you specify that the minimum version is What do you think? |
That does not work. This is the error backtrace: I guess my patch is good enough? Considering I can't afford atm to bump the ddtrace minimum version in a non-major release, supporting the 3 ways to set it up is the way to go considering the lack of a low common denominator? Would be great if the ddtrace gem considered these nuisances when breaking public interface in minor versions, in terms of providing a migration path with deprecation warning. |
So sorry. I just realised that the code snippet I gave was incorrect Could you try instead: option :error_handler do |o|
o.type :proc
o.default { Tracing::SpanOperation::Events::DEFAULT_ON_ERROR }
endIf that works the your patch could look like like: require "datadog/tracing/span_operation"
module Datadog
module Tracing
module Contrib
module Tobox
module Configuration
class Settings < Contrib::Configuration::Settings
if DDTrace::VERSION::STRING >= "1.13.0"
option :enabled do |o|
o.env "DD_TOBOX_SIDEKIQ_ENABLED"
o.type :bool
o.default true
end
option :analytics_enabled do |o|
o.env "DD_TOBOX_ANALYTICS_ENABLED"
o.type :bool
o.default false
end
option :analytics_sample_rate do |o|
o.env "DD_TRACE_TOBOX_ANALYTICS_SAMPLE_RATE"
o.type :float
o.default 1.0
end
option :service_name
option :error_handler do |o|
o.type :proc
# Workaround to avoid having to use `experimental_default_prc` or `default_proc`
o.default { Tracing::SpanOperation::Events::DEFAULT_ON_ERROR }
end
option :distributed_tracing, default: false
elsif
option :enabled do |o|
o.default { env_to_bool("DD_TOBOX_SIDEKIQ_ENABLED", true) }
o.lazy
end
option :analytics_enabled do |o|
o.default { env_to_bool("DD_TOBOX_ANALYTICS_ENABLED", false) }
o.lazy
end
option :analytics_sample_rate do |o|
o.default { env_to_float("DD_TRACE_TOBOX_ANALYTICS_SAMPLE_RATE", 1.0) }
o.lazy
end
option :service_name
option :error_handler, default: Tracing::SpanOperation::Events::DEFAULT_ON_ERROR
option :distributed_tracing, default: false
end
end
end
end
end
end
end
Yes it is
That is totally fair. Sorry for any inconvenience we caused you. We will do better next time |
|
thx. given that the workaround is viable and information is passed, I'll close the ticket. |
Current behaviour
I've had a build failure with the master version of the datadog plugin configuration of this gem I maintain. The error message doesn't show it, but the tests fail here, when loading the default error handler. I could reproduce the backtrace locally, and here it is:
Expected behaviour
I was expecting this to keep working.
I assume this was caused by this change, which is providing a path to migrate away from setting defaults like that. While that may be warranted (and should be done in a major change), it shouldn't happen at the cost of the old API breaking.
FWIW I was following changes in other adapters in order to come up with this patch, which is a bit more complicated than I would have expected to maintain as an external plugin for a 1.x series of a given library (hope what I say makes sense from your perspective).
Steps to reproduce
Load the
toboxrepo locally, use any supported ruby (you can use 3.2), set up a usable database (postgres is advised), set it up in DATABASE_URL (i.e.export DATABASE_URL=postgresql://user:pass@host/db), and run the datadog test suite (bundle exec ruby -Itest integration_tests/datadog_test.rb).The text was updated successfully, but these errors were encountered: