Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TraceSegment can outlive Tracer #89

Merged
merged 2 commits into from
Jan 15, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/datadog/datadog_agent.cpp
Original file line number Diff line number Diff line change
@@ -150,7 +150,8 @@ DatadogAgent::DatadogAgent(
const FinalizedDatadogAgentConfig& config,
const std::shared_ptr<TracerTelemetry>& tracer_telemetry,
const std::shared_ptr<Logger>& logger,
const TracerSignature& tracer_signature, ConfigManager& config_manager)
const TracerSignature& tracer_signature,
const std::shared_ptr<ConfigManager>& config_manager)
: tracer_telemetry_(tracer_telemetry),
clock_(config.clock),
logger_(logger),
2 changes: 1 addition & 1 deletion src/datadog/datadog_agent.h
Original file line number Diff line number Diff line change
@@ -66,7 +66,7 @@ class DatadogAgent : public Collector {
DatadogAgent(const FinalizedDatadogAgentConfig&,
const std::shared_ptr<TracerTelemetry>&,
const std::shared_ptr<Logger>&, const TracerSignature& id,
ConfigManager& config_manager);
const std::shared_ptr<ConfigManager>& config_manager);
~DatadogAgent();

Expected<void> send(
12 changes: 8 additions & 4 deletions src/datadog/remote_config.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "remote_config.h"

#include <cassert>
#include <cstdint>
#include <type_traits>
#include <unordered_set>
@@ -64,10 +65,13 @@ ConfigUpdate parse_dynamic_config(const nlohmann::json& j) {
} // namespace

RemoteConfigurationManager::RemoteConfigurationManager(
const TracerSignature& tracer_signature, ConfigManager& config_manager)
const TracerSignature& tracer_signature,
const std::shared_ptr<ConfigManager>& config_manager)
: tracer_signature_(tracer_signature),
config_manager_(config_manager),
client_id_(uuid()) {}
client_id_(uuid()) {
assert(config_manager_);
}

bool RemoteConfigurationManager::is_new_config(
StringView config_path, const nlohmann::json& config_meta) {
@@ -215,11 +219,11 @@ void RemoteConfigurationManager::process_response(const nlohmann::json& json) {
}

void RemoteConfigurationManager::apply_config(Configuration config) {
config_manager_.update(config.content);
config_manager_->update(config.content);
}

void RemoteConfigurationManager::revert_config(Configuration) {
config_manager_.reset();
config_manager_->reset();
}

} // namespace tracing
10 changes: 6 additions & 4 deletions src/datadog/remote_config.h
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@
// It interacts with the `ConfigManager` to seamlessly apply or revert
// configurations based on responses received from the remote source.

#include <memory>
#include <string>

#include "config_manager.h"
@@ -44,15 +45,16 @@ class RemoteConfigurationManager {
};

TracerSignature tracer_signature_;
ConfigManager& config_manager_;
std::string client_id_; ///< Identifier a `RemoteConfigurationManager`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

@dgoffredo dgoffredo Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

std::shared_ptr<ConfigManager> config_manager_;
std::string client_id_;

State state_;
std::unordered_map<std::string, Configuration> applied_config_;

public:
RemoteConfigurationManager(const TracerSignature& tracer_signature,
ConfigManager& config_manager);
RemoteConfigurationManager(
const TracerSignature& tracer_signature,
const std::shared_ptr<ConfigManager>& config_manager);

// Construct a JSON object representing the payload to be sent in a remote
// configuration request.
8 changes: 4 additions & 4 deletions src/datadog/tracer.cpp
Original file line number Diff line number Diff line change
@@ -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)),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

collector_(/* see constructor body */),
defaults_(std::make_shared<SpanDefaults>(config.defaults)),
runtime_id_(config.runtime_id ? *config.runtime_id
@@ -53,7 +54,6 @@ Tracer::Tracer(const FinalizedTracerConfig& config,
extraction_styles_(config.extraction_styles),
hostname_(config.report_hostname ? get_hostname() : nullopt),
tags_header_max_size_(config.tags_header_size),
config_manager_(config),
sampling_delegation_enabled_(config.delegate_trace_sampling) {
if (auto* collector =
std::get_if<std::shared_ptr<Collector>>(&config.collector)) {
@@ -94,7 +94,7 @@ nlohmann::json Tracer::config_json() const {
});
// clang-format on

config.merge_patch(config_manager_.config_json());
config.merge_patch(config_manager_->config_json());

if (hostname_) {
config["hostname"] = *hostname_;
@@ -122,7 +122,7 @@ Span Tracer::create_span(const SpanConfig& config) {
tracer_telemetry_->metrics().tracer.trace_segments_created_new.inc();
const auto segment = std::make_shared<TraceSegment>(
logger_, collector_, tracer_telemetry_,
config_manager_.get_trace_sampler(), span_sampler_, defaults_,
config_manager_->get_trace_sampler(), span_sampler_, defaults_,
runtime_id_, sampling_delegation_enabled_,
false /* sampling_decision_was_delegated_to_me */, injection_styles_,
hostname_, nullopt /* origin */, tags_header_max_size_,
@@ -293,7 +293,7 @@ Expected<Span> Tracer::extract_span(const DictReader& reader,
tracer_telemetry_->metrics().tracer.trace_segments_created_continued.inc();
const auto segment = std::make_shared<TraceSegment>(
logger_, collector_, tracer_telemetry_,
config_manager_.get_trace_sampler(), span_sampler_, defaults_,
config_manager_->get_trace_sampler(), span_sampler_, defaults_,
runtime_id_, sampling_delegation_enabled_, delegate_sampling_decision,
injection_styles_, hostname_, std::move(origin), tags_header_max_size_,
std::move(trace_tags), std::move(sampling_decision),
5 changes: 4 additions & 1 deletion src/datadog/tracer.h
Original file line number Diff line number Diff line change
@@ -10,6 +10,9 @@
// obtained from a `TracerConfig` via the `finalize_config` function. See
// `tracer_config.h`.

#include <cstddef>
Copy link
Collaborator

@dmehala dmehala Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#include <memory>

#include "clock.h"
#include "config_manager.h"
#include "error.h"
@@ -32,6 +35,7 @@ class SpanSampler;

class Tracer {
std::shared_ptr<Logger> logger_;
std::shared_ptr<ConfigManager> config_manager_;
std::shared_ptr<Collector> collector_;
std::shared_ptr<const SpanDefaults> defaults_;
RuntimeID runtime_id_;
@@ -44,7 +48,6 @@ class Tracer {
std::vector<PropagationStyle> extraction_styles_;
Optional<std::string> hostname_;
std::size_t tags_header_max_size_;
ConfigManager config_manager_;
bool sampling_delegation_enabled_;

public:
18 changes: 10 additions & 8 deletions test/test_remote_config.cpp
Original file line number Diff line number Diff line change
@@ -28,7 +28,8 @@ REMOTE_CONFIG_TEST("first payload") {
TracerConfig config;
config.defaults.service = "testsvc";
config.defaults.environment = "test";
ConfigManager config_manager(*finalize_config(config));
const auto config_manager =
std::make_shared<ConfigManager>(*finalize_config(config));

RemoteConfigurationManager rc(tracer_signature, config_manager);

@@ -62,7 +63,8 @@ REMOTE_CONFIG_TEST("response processing") {
config.defaults.service = "testsvc";
config.defaults.environment = "test";
config.trace_sampler.sample_rate = 1.0;
ConfigManager config_manager(*finalize_config(config));
const auto config_manager =
std::make_shared<ConfigManager>(*finalize_config(config));

RemoteConfigurationManager rc(tracer_signature, config_manager);

@@ -177,9 +179,9 @@ REMOTE_CONFIG_TEST("response processing") {

REQUIRE(!response_json.is_discarded());

const auto old_trace_sampler = config_manager.get_trace_sampler();
const auto old_trace_sampler = config_manager->get_trace_sampler();
rc.process_response(response_json);
const auto new_trace_sampler = config_manager.get_trace_sampler();
const auto new_trace_sampler = config_manager->get_trace_sampler();

CHECK(new_trace_sampler != old_trace_sampler);

@@ -201,7 +203,7 @@ REMOTE_CONFIG_TEST("response processing") {
REQUIRE(!response_json.is_discarded());

rc.process_response(response_json);
const auto current_trace_sampler = config_manager.get_trace_sampler();
const auto current_trace_sampler = config_manager->get_trace_sampler();
CHECK(old_trace_sampler == current_trace_sampler);
}

@@ -227,7 +229,7 @@ REMOTE_CONFIG_TEST("response processing") {
REQUIRE(!response_json.is_discarded());

rc.process_response(response_json);
const auto current_trace_sampler = config_manager.get_trace_sampler();
const auto current_trace_sampler = config_manager->get_trace_sampler();
CHECK(old_trace_sampler == current_trace_sampler);
}
}
@@ -270,9 +272,9 @@ REMOTE_CONFIG_TEST("response processing") {

REQUIRE(!response_json.is_discarded());

const auto old_sampling_rate = config_manager.get_trace_sampler();
const auto old_sampling_rate = config_manager->get_trace_sampler();
rc.process_response(response_json);
const auto new_sampling_rate = config_manager.get_trace_sampler();
const auto new_sampling_rate = config_manager->get_trace_sampler();

CHECK(new_sampling_rate == old_sampling_rate);
}
22 changes: 22 additions & 0 deletions test/test_trace_segment.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include <datadog/optional.h>
#include <datadog/platform_util.h>
#include <datadog/rate.h>
#include <datadog/tags.h>
@@ -446,3 +447,24 @@ TEST_CASE("TraceSegment finalization of spans") {
}
}
} // span finalizers

TEST_CASE("independent of Tracer") {
// This test verifies that a `TraceSegment` (via the `Span`s that refer to it)
// can continue to operate even after the `Tracer` that created it is
// destroyed.
//
// Primarily, the test checks that the code doesn't crash in this scenario.
TracerConfig config;
config.defaults.service = "testsvc";
config.defaults.name = "do.thing";
config.logger = std::make_shared<NullLogger>();

auto maybe_tracer = finalize_config(config);
REQUIRE(maybe_tracer);
Optional<Tracer> tracer{*maybe_tracer};

Span root = tracer->create_span();
Span child = root.create_child();

tracer.reset();
}
17 changes: 17 additions & 0 deletions test/test_tracer.cpp
Original file line number Diff line number Diff line change
@@ -23,6 +23,7 @@
#include <ctime>
#include <iosfwd>
#include <stdexcept>
#include <utility>

#include "matchers.h"
#include "mocks/collectors.h"
@@ -1702,3 +1703,19 @@ TEST_CASE("heterogeneous extraction") {

REQUIRE(writer.items == test_case.expected_injected_headers);
}

TEST_CASE("move semantics") {
// Verify that `Tracer` can be moved.
TracerConfig config;
config.defaults.service = "testsvc";
config.logger = std::make_shared<NullLogger>();
config.collector = std::make_shared<MockCollector>();

auto finalized_config = finalize_config(config);
REQUIRE(finalized_config);
Tracer tracer1{*finalized_config};

// This must compile.
Tracer tracer2{std::move(tracer1)};
(void)tracer2;
}
Loading