Skip to content

Commit

Permalink
Code review
Browse files Browse the repository at this point in the history
- Update base64 implementation to not rely on little endian.
- Move ConfigUpdate in its own heade file
dmehala committed Dec 11, 2023

Verified

This commit was signed with the committer’s verified signature. The key has expired.
dmehala Damien Mehala
1 parent 52375e0 commit f105c8f
Showing 8 changed files with 45 additions and 38 deletions.
1 change: 1 addition & 0 deletions BUILD.bazel
Original file line number Diff line number Diff line change
@@ -52,6 +52,7 @@ cc_library(
"src/datadog/cerr_logger.h",
"src/datadog/clock.h",
"src/datadog/config_manager.h",
"src/datadog/config_update.h",
"src/datadog/collector.h",
"src/datadog/collector_response.h",
# "src/datadog/curl.h", no libcurl
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -158,6 +158,7 @@ target_sources(dd_trace_cpp-objects PUBLIC
src/datadog/cerr_logger.h
src/datadog/clock.h
src/datadog/config_manager.h
src/datadog/config_update.h
src/datadog/collector.h
src/datadog/collector_response.h
# src/datadog/curl.h except for curl.h
40 changes: 15 additions & 25 deletions src/datadog/base64.cpp
Original file line number Diff line number Diff line change
@@ -11,7 +11,7 @@ constexpr uint8_t _ = k_sentinel; // for brevity
constexpr uint8_t k_eol = 0;

// Invalid inputs are mapped to the value 255. '=' maps to 0.
constexpr uint8_t k_base64_table[]{
constexpr uint8_t k_base64_table[] = {
_, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _,
_, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _,
_, _, _, _, _, _, _, 62, _, _, _, 63, 52, 53, 54, 55, 56, 57,
@@ -34,11 +34,6 @@ std::string base64_decode(StringView input) {
std::string output;
output.reserve(in_size);

union {
uint32_t buffer;
uint8_t bytes[4];
} decoder;

size_t i = 0;

for (; i + 4 < in_size;) {
@@ -52,17 +47,9 @@ std::string base64_decode(StringView input) {
return "";
}

decoder.buffer = 0 | c0 << 26 | c1 << 20 | c2 << 14 | c3 << 8;

// NOTE(@dmehala): It might seem confusing to read those bytes input reverse
// order. It is related to the architecture endianess. For now the set of
// architecture we support (x86_64 and arm64) are all little-endian.
// TODO(@dgoffredo): I'd prefer an endian-agnostic implementation.
// nginx-datadog targets x86_64 and arm64 in its binary releases, but
// dd-trace-cpp targets any standard C++17 compiler.
output.push_back(decoder.bytes[3]);
output.push_back(decoder.bytes[2]);
output.push_back(decoder.bytes[1]);
output.push_back(c0 << 2 | (c1 & 0xF0) >> 4);
output.push_back((c1 & 0x0F) << 4 | ((c2 & 0x3C) >> 2));
output.push_back(((c2 & 0x03) << 6) | (c3 & 0x3F));
}

// If padding is missing, return the empty string in lieu of an Error.
@@ -78,17 +65,20 @@ std::string base64_decode(StringView input) {
return "";
}

decoder.buffer = 0 | c0 << 26 | c1 << 20 | c2 << 14 | c3 << 8;

if (c2 == k_eol) {
output.push_back(decoder.bytes[3]);
// The last quadruplet is of the form "xx==", where only one character needs
// to be decoded.
output.push_back(c0 << 2 | (c1 & 0xF0) >> 4);
} else if (c3 == k_eol) {
output.push_back(decoder.bytes[3]);
output.push_back(decoder.bytes[2]);
// The last quadruplet is of the form "xxx=", where only two character needs
// to be decoded.
output.push_back(c0 << 2 | (c1 & 0xF0) >> 4);
output.push_back((c1 & 0x0F) << 4 | ((c2 & 0x3C) >> 2));
} else {
output.push_back(decoder.bytes[3]);
output.push_back(decoder.bytes[2]);
output.push_back(decoder.bytes[1]);
// The last quadruplet is not padded -> common use case
output.push_back(c0 << 2 | (c1 & 0xF0) >> 4);
output.push_back((c1 & 0x0F) << 4 | ((c2 & 0x3C) >> 2));
output.push_back(((c2 & 0x03) << 6) | (c3 & 0x3F));
}

return output;
2 changes: 1 addition & 1 deletion src/datadog/config_manager.cpp
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@ std::shared_ptr<TraceSampler> ConfigManager::get_trace_sampler() {
return current_trace_sampler_;
}

void ConfigManager::update(const ConfigManager::Update& conf) {
void ConfigManager::update(const ConfigUpdate& conf) {
std::lock_guard<std::mutex> lock(mutex_);

if (conf.trace_sampler) {
8 changes: 2 additions & 6 deletions src/datadog/config_manager.h
Original file line number Diff line number Diff line change
@@ -5,8 +5,8 @@
#include <mutex>

#include "clock.h"
#include "config_update.h"
#include "json.hpp"
#include "trace_sampler.h"
#include "tracer_config.h"

namespace datadog {
@@ -19,17 +19,13 @@ class ConfigManager {
std::shared_ptr<TraceSampler> current_trace_sampler_;

public:
struct Update {
Optional<TraceSamplerConfig> trace_sampler;
};

ConfigManager(const FinalizedTracerConfig& config);

// Return the `TraceSampler` consistent with the most recent configuration.
std::shared_ptr<TraceSampler> get_trace_sampler();

// Apply the specified `conf` update.
void update(const Update& conf);
void update(const ConfigUpdate& conf);

// Restore the configuration that was passed to this object's constructor,
// overriding any previous calls to `update`.
13 changes: 13 additions & 0 deletions src/datadog/config_update.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#include "optional"
#include "trace_sampler.h"

namespace datadog {
namespace tracing {

// TODO: Document
struct ConfigUpdate {
Optional<TraceSamplerConfig> trace_sampler;
};

} // namespace tracing
} // namespace datadog
16 changes: 11 additions & 5 deletions src/datadog/remote_config.cpp
Original file line number Diff line number Diff line change
@@ -5,6 +5,7 @@
#include <unordered_set>

#include "base64.h"
#include "datadog/string_view.h"
#include "json.hpp"
#include "random.h"
#include "version.h"
@@ -43,20 +44,25 @@ constexpr std::array<uint8_t, sizeof(uint64_t)> capabilities_byte_array(
constexpr std::array<uint8_t, sizeof(uint64_t)> k_apm_capabilities =
capabilities_byte_array(APM_TRACING_SAMPLE_RATE);

constexpr StringView k_apm_product_path_substring = "/APM_TRACING/";
constexpr StringView k_apm_product = "APM_TRACING";
constexpr StringView k_apm_product_path_substring = "/APM_TRACING/";

} // namespace
ConfigUpdate parse_dynamic_config(const nlohmann::json& j) {
ConfigUpdate config_update;

void from_json(const nlohmann::json& j, ConfigManager::Update& out) {
if (auto sampling_rate_it = j.find("tracing_sampling_rate");
sampling_rate_it != j.cend()) {
TraceSamplerConfig trace_sampler_cfg;
trace_sampler_cfg.sample_rate = *sampling_rate_it;
out.trace_sampler = trace_sampler_cfg;

config_update.trace_sampler = trace_sampler_cfg;
}

return config_update;
}

} // namespace

RemoteConfigurationManager::RemoteConfigurationManager(
const TracerID& tracer_id, ConfigManager& config_manager)
: tracer_id_(tracer_id),
@@ -182,7 +188,7 @@ void RemoteConfigurationManager::process_response(const nlohmann::json& json) {
new_config.hash = config_metadata.at("/hashes/sha256"_json_pointer);
new_config.id = config_json.at("id");
new_config.version = config_json.at("revision");
new_config.content = ConfigManager::Update(config_json.at("lib_config"));
new_config.content = parse_dynamic_config(config_json.at("lib_config"));

apply_config(new_config);
applied_config_[std::string{config_path}] = new_config;
2 changes: 1 addition & 1 deletion src/datadog/remote_config.h
Original file line number Diff line number Diff line change
@@ -26,7 +26,7 @@ class RemoteConfigurationManager {
std::string id;
std::string hash;
std::size_t version;
ConfigManager::Update content;
ConfigUpdate content;
};

TracerID tracer_id_;

0 comments on commit f105c8f

Please sign in to comment.