Skip to content

Allow setting internal tags and expose numeric tags as metrics #111

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 15, 2024
Merged

Allow setting internal tags and expose numeric tags as metrics #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dbed898
Allow setting internal tags and expose numeric tags as metrics
Anilm3 Apr 9, 2024
3f6bc2a
Fix format
Anilm3 Apr 10, 2024
593a07a
Simplify tests
Anilm3 Apr 11, 2024
98fa63c
use metric in sys-test server
dmehala Apr 15, 2024
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
26 changes: 16 additions & 10 deletions src/datadog/span.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,27 +101,33 @@ const std::string& Span::name() const { return data_->name; }
const std::string& Span::resource_name() const { return data_->resource; }

Optional<StringView> Span::lookup_tag(StringView name) const {
if (tags::is_internal(name)) {
const auto found = data_->tags.find(std::string(name));
if (found == data_->tags.end()) {
return nullopt;
}
return found->second;
}

const auto found = data_->tags.find(std::string(name));
if (found == data_->tags.end()) {
Optional<double> Span::lookup_metric(StringView name) const {
const auto found = data_->numeric_tags.find(std::string(name));
if (found == data_->numeric_tags.end()) {
return nullopt;
}
return found->second;
}

void Span::set_tag(StringView name, StringView value) {
if (!tags::is_internal(name)) {
data_->tags.insert_or_assign(std::string(name), std::string(value));
}
data_->tags.insert_or_assign(std::string(name), std::string(value));
}

void Span::remove_tag(StringView name) {
if (!tags::is_internal(name)) {
data_->tags.erase(std::string(name));
}
void Span::set_metric(StringView name, double value) {
data_->numeric_tags.insert_or_assign(std::string(name), value);
}

void Span::remove_tag(StringView name) { data_->tags.erase(std::string(name)); }

void Span::remove_metric(StringView name) {
data_->numeric_tags.erase(std::string(name));
}

void Span::set_service_name(StringView service) {
Expand Down
9 changes: 8 additions & 1 deletion src/datadog/span.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,19 @@ class Span {
// Return the value of the tag having the specified `name`, or return null if
// there is no such tag.
Optional<StringView> lookup_tag(StringView name) const;
// Return the value of the metric having the specified `name`, or return null
// if there is no such metric.
Optional<double> lookup_metric(StringView name) const;
// Overwrite the tag having the specified `name` so that it has the specified
// `value`, or create a new tag.
void set_tag(StringView name, StringView value);
// Overwrite the metric having the specified `name` so that it has the
// specified `value`, or create a new metric.
void set_metric(StringView name, double value);
// Delete the tag having the specified `name` if it exists.
void remove_tag(StringView name);

// Delete the metric having the specified `name` if it exists.
void remove_metric(StringView name);
// Set the name of the service associated with this span, e.g.
// "ingress-nginx-useast1".
void set_service_name(StringView);
Expand Down
6 changes: 2 additions & 4 deletions src/datadog/span_data.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,7 @@ void SpanData::apply_config(const SpanDefaults& defaults,
tags.insert_or_assign(tags::version, version);
}
for (const auto& [key, value] : config.tags) {
if (!tags::is_internal(key)) {
tags.insert_or_assign(key, value);
}
Comment on lines -54 to -56
Copy link
Contributor

@dgoffredo dgoffredo Apr 11, 2024

Choose a reason for hiding this comment

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

Do you want any user of the library to be able to set internal tags? Same question in span.cpp.

If Datadog has an internal client who wishes to have special access to the span representation, then consider adding an API specific to that use case, and document it as such.

Copy link
Contributor Author

@Anilm3 Anilm3 Apr 12, 2024

Choose a reason for hiding this comment

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

I'm not sure there is much value in forbidding internal tags, however I'm open to the idea of separating the *_tag and *_metric methods into *_internal_tag and *internal_metric if that makes it more palatable.

tags.insert_or_assign(key, value);
}

resource = config.resource.value_or(name);
Expand Down Expand Up @@ -139,4 +137,4 @@ Expected<void> msgpack_encode(
}

} // namespace tracing
} // namespace datadog
} // namespace datadog
20 changes: 18 additions & 2 deletions test/system-tests/request_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,24 @@ void RequestHandler::on_set_meta(const httplib::Request& req,

void RequestHandler::on_set_metric(const httplib::Request& /* req */,
httplib::Response& res) {
// No method available for directly setting a span metric.
// Returning OK instead of UNIMPLEMENTED to satisfy the test framework.
const auto request_json = nlohmann::json::parse(res.body);

auto span_id = utils::get_if_exists<uint64_t>(request_json, "span_id");
if (!span_id) {
VALIDATION_ERROR(res, "on_set_meta: missing `span_id` field.");
}

auto span_it = active_spans_.find(*span_id);
if (span_it == active_spans_.cend()) {
const auto msg =
"on_set_meta: span not found for id " + std::to_string(*span_id);
VALIDATION_ERROR(res, msg);
}

auto& span = span_it->second;
span.set_metric(request_json.at("key").get<std::string_view>(),
request_json.at("value").get<double>());

res.status = 200;
}

Expand Down
171 changes: 136 additions & 35 deletions test/test_span.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ TEST_CASE("set_tag") {
span.set_tag("foo", "lemon");
span.set_tag("foo.bar", "mint");
span.set_tag("foo.baz", "blueberry");
span.set_tag("_dd.secret.sauce", "thousand islands");
span.set_tag("_dd_not_internal", "");
span.set_tag("_dd.chipmunk", "");
}

REQUIRE(collector->chunks.size() == 1);
Expand All @@ -57,18 +60,21 @@ TEST_CASE("set_tag") {
REQUIRE(span.tags.at("foo") == "lemon");
REQUIRE(span.tags.at("foo.bar") == "mint");
REQUIRE(span.tags.at("foo.baz") == "blueberry");
REQUIRE(span.tags.at("_dd.secret.sauce") == "thousand islands");
REQUIRE(span.tags.at("_dd_not_internal") == "");
REQUIRE(span.tags.at("_dd.chipmunk") == "");
}

SECTION("tags can be overwritten") {
{
SpanConfig config;
config.tags = {
{"color", "purple"},
{"turtle.depth", "all the way down"},
};
config.tags = {{"color", "purple"},
{"turtle.depth", "all the way down"},
{"_dd.tag", "written"}};
auto span = tracer.create_span(config);
span.set_tag("color", "green");
span.set_tag("bonus", "applied");
span.set_tag("_dd.tag", "overwritten");
}

REQUIRE(collector->chunks.size() == 1);
Expand All @@ -80,29 +86,7 @@ TEST_CASE("set_tag") {
REQUIRE(span.tags.at("color") == "green");
REQUIRE(span.tags.at("turtle.depth") == "all the way down");
REQUIRE(span.tags.at("bonus") == "applied");
}

SECTION("can't set internal tags directly") {
{
auto span = tracer.create_span();
span.set_tag("foo", "lemon");
span.set_tag("_dd.secret.sauce", "thousand islands");
span.set_tag("_dd_not_internal", "");
// _dd.p.dm will end up in the tags due to how sampling works
// span.set_tag("_dd.p.dm", "-4");
span.set_tag("_dd.chipmunk", "");
}

REQUIRE(collector->chunks.size() == 1);
const auto& chunk = collector->chunks.front();
REQUIRE(chunk.size() == 1);
const auto& span_ptr = chunk.front();
REQUIRE(span_ptr);
const auto& span = *span_ptr;
REQUIRE(span.tags.at("foo") == "lemon");
REQUIRE(span.tags.count("_dd.secret.sauce") == 0);
REQUIRE(span.tags.at("_dd_not_internal") == "");
REQUIRE(span.tags.count("_dd.chipmunk") == 0);
REQUIRE(span.tags.at("_dd.tag") == "overwritten");
}
}

Expand All @@ -120,34 +104,32 @@ TEST_CASE("lookup_tag") {
auto span = tracer.create_span();
REQUIRE(!span.lookup_tag("nope"));
REQUIRE(!span.lookup_tag("also nope"));
REQUIRE(!span.lookup_tag("_dd.nope"));
}

SECTION("lookup after set") {
auto span = tracer.create_span();
span.set_tag("color", "purple");
span.set_tag("turtle.depth", "all the way down");
span.set_tag("_dd.tag", "found");

REQUIRE(span.lookup_tag("color") == "purple");
REQUIRE(span.lookup_tag("turtle.depth") == "all the way down");
REQUIRE(span.lookup_tag("_dd.tag") == "found");
}

SECTION("lookup after config") {
SpanConfig config;
config.tags = {
{"color", "purple"},
{"turtle.depth", "all the way down"},
{"_dd.tag", "found"},
};
auto span = tracer.create_span(config);

REQUIRE(span.lookup_tag("color") == "purple");
REQUIRE(span.lookup_tag("turtle.depth") == "all the way down");
}

SECTION("internal tags redacted") {
auto span = tracer.create_span();
REQUIRE(!span.lookup_tag("_dd.this"));
REQUIRE(!span.lookup_tag("_dd.that"));
REQUIRE(!span.lookup_tag("_dd.the.other.thing"));
REQUIRE(span.lookup_tag("_dd.tag") == "found");
}
}

Expand All @@ -164,22 +146,141 @@ TEST_CASE("remove_tag") {
SECTION("doesn't have to be there already") {
auto span = tracer.create_span();
span.remove_tag("not even there");
span.remove_tag("_dd.tag");
}

SECTION("after removal, lookup yields null") {
SpanConfig config;
config.tags = {{"mayfly", "carpe diem"}};
config.tags = {{"mayfly", "carpe diem"}, {"_dd.mayfly", "carpe diem"}};
auto span = tracer.create_span(config);
span.set_tag("foo", "bar");

span.remove_tag("mayfly");
span.remove_tag("_dd.mayfly");
span.remove_tag("foo");

REQUIRE(!span.lookup_tag("mayfly"));
REQUIRE(!span.lookup_tag("_dd.mayfly"));
REQUIRE(!span.lookup_tag("foo"));
}
}

TEST_CASE("set_metric") {
TracerConfig config;
config.service = "testsvc";
const auto collector = std::make_shared<MockCollector>();
config.collector = collector;
config.logger = std::make_shared<MockLogger>();

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

SECTION("metrics end up in the collector") {
{
auto span = tracer.create_span();
span.set_metric("foo", 5.0);
span.set_metric("foo.bar", 3.0);
span.set_metric("foo.baz", 1.0);
span.set_metric("_dd.secret.sauce", 2.0);
span.set_metric("_dd_not_internal", 3.0);
span.set_metric("_dd.chipmunk", 4.0);
}

REQUIRE(collector->chunks.size() == 1);
const auto& chunk = collector->chunks.front();
REQUIRE(chunk.size() == 1);
const auto& span_ptr = chunk.front();
REQUIRE(span_ptr);
const auto& span = *span_ptr;
REQUIRE(span.numeric_tags.at("foo") == 5.0);
REQUIRE(span.numeric_tags.at("foo.bar") == 3.0);
REQUIRE(span.numeric_tags.at("foo.baz") == 1.0);
REQUIRE(span.numeric_tags.at("_dd.secret.sauce") == 2.0);
REQUIRE(span.numeric_tags.at("_dd_not_internal") == 3.0);
REQUIRE(span.numeric_tags.at("_dd.chipmunk") == 4.0);
}

SECTION("metrics can be overwritten") {
{
auto span = tracer.create_span();
span.set_metric("color", 2.0);
span.set_metric("color", 1.0);
span.set_metric("bonus", 6.0);
span.set_metric("bonus", 5.0);
}

REQUIRE(collector->chunks.size() == 1);
const auto& chunk = collector->chunks.front();
REQUIRE(chunk.size() == 1);
const auto& span_ptr = chunk.front();
REQUIRE(span_ptr);
const auto& span = *span_ptr;
REQUIRE(span.numeric_tags.at("color") == 1.0);
REQUIRE(span.numeric_tags.at("bonus") == 5.0);
}
}

TEST_CASE("lookup_metric") {
TracerConfig config;
config.service = "testsvc";
config.collector = std::make_shared<MockCollector>();
config.logger = std::make_shared<MockLogger>();

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

SECTION("not found is null") {
auto span = tracer.create_span();
REQUIRE(!span.lookup_metric("nope"));
REQUIRE(!span.lookup_metric("also nope"));
REQUIRE(!span.lookup_metric("_dd.nope"));
}

SECTION("lookup after set") {
auto span = tracer.create_span();
span.set_metric("color", 11.0);
span.set_metric("turtle.depth", 6.0);
span.set_metric("_dd.this", 33.0);

REQUIRE(span.lookup_metric("color") == 11.0);
REQUIRE(span.lookup_metric("turtle.depth") == 6.0);
REQUIRE(span.lookup_metric("_dd.this") == 33.0);
}
}

TEST_CASE("remove_metric") {
TracerConfig config;
config.service = "testsvc";
config.collector = std::make_shared<MockCollector>();
config.logger = std::make_shared<MockLogger>();

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

SECTION("doesn't have to be there already") {
auto span = tracer.create_span();
span.remove_metric("not even there");
}

SECTION("after removal, lookup yields null") {
auto span = tracer.create_span();
span.set_metric("mayfly", 10.0);
span.set_metric("foo", 11.0);
span.set_metric("_dd.metric", 1.0);

span.remove_metric("mayfly");
span.remove_metric("foo");
span.remove_metric("_dd.metric");

REQUIRE(!span.lookup_metric("mayfly"));
REQUIRE(!span.lookup_metric("foo"));
REQUIRE(!span.lookup_metric("_dd.metric"));
}
}

TEST_CASE("span duration") {
TracerConfig config;
config.service = "testsvc";
Expand Down