feat: support DD_TRACE_ENABLED dynamic configuration#96
Conversation
dgoffredo
left a comment
There was a problem hiding this comment.
This technique looks good. I like how it's consistent with previous use of ConfigManager.
There is the question, that you asked internally, of whether already-started traces should be allowed to be sent. A design where the answer is "no" might be better from a user perspective, but the implementation would be less elegant.
Please fix the test failure showing in CI, and also please add tests for the new behavior.
061e775 to
139446f
Compare
| // Return the `SpanDefaults` consistent with the most recent configuration. | ||
| std::shared_ptr<const SpanDefaults> get_span_defaults(); | ||
|
|
||
| bool get_report_traces(); |
There was a problem hiding this comment.
- document (and rename?)
There was a problem hiding this comment.
We could follow the lead of TracerConfig and omit the get_ prefix. It's fine either way.
Yes, please do add a comment here. Maybe:
// Return whether spans should be sent to the collector.
I don't like the use of the word "should." Something like that, anyway.
BenchmarksBenchmark execution time: 2024-02-01 11:04:05 Comparing candidate commit 8ad3bd3 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
| // Return the `SpanDefaults` consistent with the most recent configuration. | ||
| std::shared_ptr<const SpanDefaults> get_span_defaults(); | ||
|
|
||
| bool get_report_traces(); |
There was a problem hiding this comment.
We could follow the lead of TracerConfig and omit the get_ prefix. It's fine either way.
Yes, please do add a comment here. Maybe:
// Return whether spans should be sent to the collector.
I don't like the use of the word "should." Something like that, anyway.
| if (tracing_enabled_it->is_boolean()) { | ||
| config_update.report_traces = tracing_enabled_it->get<bool>(); | ||
| } else { | ||
| // TODO: report to telemetry |
No description provided.