TraceSegment can outlive Tracer #89
Conversation
It looks like there's a typo at the beginning, and I couldn't think of a good way to describe `client_id` concisely, so probably best to omit the comment.
| @@ -36,6 +36,7 @@ Tracer::Tracer(const FinalizedTracerConfig& config) | |||
| Tracer::Tracer(const FinalizedTracerConfig& config, | |||
| const std::shared_ptr<const IDGenerator>& generator) | |||
| : logger_(config.logger), | |||
| config_manager_(std::make_shared<ConfigManager>(config)), | |||
There was a problem hiding this comment.
I moved this up so that it occurs before collector_. This is not at all necessary, since we're talking about shared pointers, but it mirrors what we would do if the collector and the config manager were both held by value.
BenchmarksBenchmark execution time: 2024-01-12 20:07:58 Comparing candidate commit 8de1e1c in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
| @@ -44,15 +45,16 @@ class RemoteConfigurationManager { | |||
| }; | |||
|
|
|||
| TracerSignature tracer_signature_; | |||
| ConfigManager& config_manager_; | |||
| std::string client_id_; ///< Identifier a `RemoteConfigurationManager` | |||
There was a problem hiding this comment.
I figured the comment contained a typo:
///< Identifier a `RemoteConfigurationManager`
I'm not sure what the extra /< is doing, and "Identifier" should probably be "Identifies".
Ok, suppose the comment is "Identifies a RemoteConfigurationManager." True enough, but that doesn't really help me understand what client_id_ is or is for. So, I thought about how best to describe client_id_ in a comment. I didn't think of anything that was better than nothing, so I went with nothing.
| @@ -10,6 +10,9 @@ | |||
| // obtained from a `TracerConfig` via the `finalize_config` function. See | |||
| // `tracer_config.h`. | |||
|
|
|||
| #include <cstddef> | |||
There was a problem hiding this comment.
Since we use std::size_t explicitly, I figured to include a header for it. Could include string as well, but I didn't notice that one.
|
Thanks for the review. I'll log off after this merge. It's a USA holiday today :D |
I was integrating the recent sampling delegation changes into the corresponding feature branch in nginx-datadog, and got some compiler errors. They're due to other changes on the dd-trace-cpp main branch.
In nginx-datadog, and in Envoy, the
Traceris stored as anOptional<Tracer>. ATracerinstance is then moved into the variable.Since
Tracernow has aConfigManagerdata member, andConfigManagerhas astd::mutexdata member,Tracercan no longer be moved (becausestd::mutexcannot be moved).One thing I could do is use
Optional::emplaceto construct theTracerin place: no move required.Another thing I could do is make the
Tracer'sConfigManagerastd::unique_ptr, so that move semantics are preserved.Yet another thing I could do is use a
std::unique_ptrin nginx-datadog and Envoy, so that theTraceritself does not have to be moved.So, I went about thinking about this. Then, looking at the dd-trace-cpp code, I noticed something.
TracerhasConfigManagerdata member. A reference (pointer) to theConfigManageris then passed into theDatadogAgent, which is held byshared_ptrinTracer. TheDatadogAgenthas aRemoteConfigurationManagerdata member, which holds a reference (pointer) to theConfigManager.My initial design of dd-trace-cpp was such that a
TraceSegmentcould safely outlive theTracerthat created it. The idea was that an application might have aTracerthat gets reassigned when reconfiguration occurs, and this would not affect any traces in flight.The recent remote configuration changes violate this property, because the lifetime of the
ConfigManageris limited to the lifetime of theTracerthat holds it, even though theConfigManageris referred to by objects that can outlive theTracer.This pull request makes the
Tracer'sConfigManagerastd::shared_ptr, and passes it as ashared_ptrthroughDatadogAgentand intoRemoteConfigurationManager. This way, even if theTraceris destroyed, theRemoteConfigurationManagerstill has a valid handle to theConfigManager.