Extract components from configuration#996
Conversation
delner
added
core
|
I want to double check that |
| else | ||
| Datadog::ContextFlush::Finished.new | ||
| end | ||
| if options.key?(:partial_flush) |
There was a problem hiding this comment.
This was a bug where calling #configure would turn off partial flushing if it had previously been configured, but the option wasn't specified on the second call.
| priority_sampling = options.fetch(:priority_sampling, nil) | ||
| writer = options.fetch(:writer, nil) | ||
| transport_options = options.fetch(:transport_options, {}) | ||
| transport_options = options.fetch(:transport_options, {}).dup |
There was a problem hiding this comment.
This was a bug where building options for a new writer would mutate the provided Hash by reference, causing unwanted side effects.
| RSpec.describe Datadog::Configuration::Settings do | ||
| subject(:settings) { described_class.new } | ||
|
|
||
| describe '#analytics' do |
There was a problem hiding this comment.
Backfilled a lot of tests for configuration settings in this file that was previously missing.
| after do | ||
| Datadog::Logger.debug_logging = false | ||
| context 'when modified' do | ||
| it 'does not modify the default by reference' do |
There was a problem hiding this comment.
This was an issue when option :writer_options, default: {} was defined, because a user could errantly modify the default by reference.
| @@ -9,6 +10,140 @@ | |||
|
|
|||
| describe '#configure' do | |||
There was a problem hiding this comment.
Most of these tests are "integration-like" in the sense that they apply some high level settings and consequently run through other components like Configuration::Settings and Configuration::Components and assert on the stateful outcome of those classes.
Might want to extract this kind of behavioral test elsewhere, but put it in here for now, to ensure basic usage works as intended.
marcotc
left a comment
marcotc
left a comment
There was a problem hiding this comment.
Very nice improvements, looks great overall!
There are only few questions I asked in my comments below.
2 participants
Currently our configuration settings and tracer components (
Tracer,RuntimeMetrics, etc) are mutable which makes state management cumbersome and can result in some undesirable side effects when reconfigured.In this pull request, we'd like to address some of these issues by moving towards an immutable configuration model, where tracer components are not mutated, but recreated. This should make state management of tracer components simpler and consistent.
In practice:
Datadog.configure { |c| #... }and apply the settings they desire, which mutates theDatadog.configurationsettings object.Datadog.configureblock completes, it creates aComponentsobject from the settings, which contains all the major components used by the tracer (e.g.Tracer,HealthMetrics, etc.)Componentsare torn down (e.g. clean up threads, etc), then replaced by the newComponentsinstance.Datadog.tracerfor use.To accomplish this, it requires moving a lot of parts around, including that used to be inside
Datadog::Configuration::SettingstoDatadog::Configuration::Components, including:Tracer,RuntimeMetrics,HealthMetricsfromDatadog::Configuration::SettingstoDatadog::Configuration::Componentsc.tracer { enabled: true, ... }-->c.tracer.enabled = trueand more.c.runtime_metrics { enabled: true, ... }-->c.runtime_metrics.enabled = trueand more.c.analytics_enabled = true-->c.analytics.enabled = true