8000 error tags (#244) · DataDog/dd-opentracing-cpp@7968076 · GitHub
[go: up one dir, main page]
Skip to content

Commit 7968076

Browse files
dgoffredodgoffredo
authored
error tags (#244)
* add a test that fails * new test passes * clang-format * revise the "error" tag logic further
1 parent c9b3c90 commit 7968076

File tree

4 files changed

+200
-34
lines changed

4 files changed

+200
-34
lines changed

‎src/bool.cpp‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
#include <string>
1+
#include "bool.h"
2+
23
#include <unordered_map>
34

45
namespace datadog {
@@ -12,17 +13,27 @@ std::unordered_map<std::string, bool> conversions{
1213
} // namespace
1314

1415
bool stob(const std::string& str, bool fallback) {
15-
if (str.empty()) {
16-
return fallback;
17-
}
18-
auto result = conversions.find(str);
19-
if (result == conversions.end()) {
20-
return fallback;
16+
switch (tribool(str)) {
17+
case Tribool::True:
18+
return true;
19+
case Tribool::False:
20+
return false;
21+
default:
22+
return fallback;
2123
}
22-
return result->second;
2324
}
2425

25-
bool isbool(const std::string& str) { return conversions.find(str) != conversions.end(); }
26+
bool isbool(const std::string& str) { return tribool(str) != Tribool::Neither; }
27+
28+
Tribool tribool(bool value) { return value ? Tribool::True : Tribool::False; }
29+
30+
Tribool tribool(const std::string& str) {
31+
auto entry = conversions.find(str);
32+
if (entry == conversions.end()) {
33+
return Tribool::Neither;
34+
}
35+
return tribool(entry->second);
36+
}
2637

2738
} // namespace opentracing
2839
} // namespace datadog

‎src/bool.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,19 @@
11
#ifndef DD_OPENTRACING_BOOL_H
22
#define DD_OPENTRACING_BOOL_H
33

4+
#include <string>
5+
46
namespace datadog {
57
namespace opentracing {
68

79
bool stob(const std::string& str, bool fallback);
810
bool isbool(const std::string& str);
911

12+
enum class Tribool { False, True, Neither };
13+
14+
Tribool tribool(const std::string&);
15+
Tribool tribool(bool);
16+
1017
} // namespace opentracing
1118
} // namespace datadog
1219

‎src/span.cpp‎

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,45 @@ std::regex &PATH_MIXED_ALPHANUMERICS() {
103103
"\\d\\?]*[\\d-]+[^\\/]*))"};
104104
return r;
105105
}
106+
107+
// Deduce `span.error` and "error*" tag values from the set values of "error*"
108+
// tags.
109+
// See the error-related test SECTIONs in `span_test.cpp` for more information.
110+
void finish_error_tags(SpanData &span) {
111+
const std::string error_details[] = {"error.msg", "error.stack", "error.type"};
112+
const auto error_tag = span.meta.find(::ot::ext::error);
113+
114+
if (error_tag != span.meta.end()) {
115+
const auto &value = error_tag->second;
116+
const Tribool boolness = tribool(value);
117+
118+
if (value.empty() || boolness == Tribool::False) {
119+
span.error = 0;
120+
span.meta.erase(error_tag);
121+
for (const auto &tag_name : error_details) {
122+
span.meta.erase(tag_name);
123+
}
124+
return;
125+
}
126+
127+
span.error = 1;
128+
if (boolness == Tribool::Neither) {
129+
span.meta.emplace("error.msg", value);
130+
}
131+
span.meta.erase(error_tag);
132+
return;
133+
}
134+
135+
for (const auto &tag_name : error_details) {
136+
if (span.meta.count(tag_name)) {
137+
span.error = 1;
138+
if (error_tag != span.meta.end()) {
139+
span.meta.erase(error_tag);
140+
}
141+
return;
142+
}
143+
}
144+
}
106145
} // namespace
107146

108147
// Imperfectly audits the data in a Span, removing some things that could cause information leaks
@@ -134,7 +173,6 @@ void Span::FinishWithOptions(
134173
span_->duration =
135174
std::chrono::duration_cast<std::chrono::nanoseconds>(end_time - start_time_).count();
136175
// Apply special tags.
137-
// If we add any more cases; then abstract this. For now, KISS.
138176
auto tag = span_->meta.find(tags::span_type);
139177
if (tag != span_->meta.end()) {
140178
span_->type = tag->second;
@@ -150,19 +188,7 @@ void Span::FinishWithOptions(
150188
span_->service = tag->second;
151189
span_->meta.erase(tag);
152190
}
153-
tag = span_->meta.find(::ot::ext::error);
154-
if (tag != span_->meta.end()) {
155-
// tag->second is the JSON-serialized value of the variadic type given to SetTag.
156-
// Errors can be a flag or a detailed message.
157-
// Empty or false-y values indicate no error.
158-
// Any other value will mark the span to indicate an error occured.
159-
if (tag->second == "" || !stob(tag->second, true)) {
160-
span_->error = 0;
161-
} else {
162-
span_->error = 1;
163-
}
164-
// Don't erase the tag, in case it is populated with interesting information.
165-
}
191+
finish_error_tags(*span_);
166192
tag = span_->meta.find(tags::analytics_event);
167193
if (tag != span_->meta.end()) {
168194
// tag->second is the JSON-serialized value of the variadic type given to SetTag.

‎test/span_test.cpp‎

Lines changed: 133 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -306,19 +306,20 @@ TEST_CASE("span") {
306306
std::string span_tag;
307307
};
308308

309+
// The resulting "error" tag is always unset.
310+
// See the next section for more information.
309311
auto error_tag_test_case = GENERATE(values<ErrorTagTestCase>({
310-
{"0", 0, "0"},
311-
{0, 0, "0"},
312+
{"0", 0, ""},
313+
{0, 0, ""},
312314
{"", 0, ""},
313-
{"false", 0, "false"},
314-
{false, 0, "false"},
315-
{"1", 1, "1"},
316-
{1, 1, "1"},
317-
{"any random truth-ish string or value lol", 1,
318-
"any random truth-ish string or value lol"},
319-
{std::vector<ot::Value>{"hi", 420, true}, 1, "[\"hi\",420,true]"},
320-
{"true", 1, "true"},
321-
{true, 1, "true"},
315+
{"false", 0, ""},
316+
{false, 0, ""},
317+
{"1", 1, ""},
318+
{1, 1, ""},
319+
{"any random truth-ish string or value lol", 1, ""},
320+
{std::vector<ot::Value>{"hi", 420, true}, 1, ""},
321+
{"true", 1, ""},
322+
{true, 1, ""},
322323
}));
323324

324325
span.SetTag("error", error_tag_test_case.value);
@@ -329,6 +330,127 @@ TEST_CASE("span") {
329330
REQUIRE(result->meta["error"] == error_tag_test_case.span_tag);
330331
}
331332

333+
SECTION("error.* tags override error tag") {
334+
// The tag name `opentracing::ext::error` is "error", which is also the
335+
// first part of the nested tags "error.msg", "error.stack", and
336+
// "error.type". The latter tags are significant to Error Tracking
337+
// <https://docs.datadoghq.com/tracing/error_tracking/>.
338+
//
339+
// It was found that setting the "error" tag makes some parts of Datadog
340+
// behave as if all "error.*" tags had been removed. A user who set both
341+
// the "error" tag and the "error.msg" tag, for example, might find that
342+
// only the "error" tag appeared in the Datadog UI.
343+
//
344+
// This test section verifies that the above does not happen.
345+
346+
auto span_id = get_id();
347+
Span span{logger, nullptr, buffer, get_time,
348+
span_id, span_id, 0, SpanContext{logger, span_id, span_id, "", {}},
349+
get_time(), "", "", "",
350+
"", ""};
351+
352+
using OptionalString = ot::util::variant<std::nullptr_t, std::string>;
353+
354+
// For each member of `ErrorTags`, `nullptr` denotes the absence of the tag.
355+
struct ErrorTags {
356+
OptionalString error; // "error" tag
357+
OptionalString msg; // "error.msg" tag
358+
OptionalString stack; // "error.stack" tag
359+
OptionalString type; // "error.type" tag
360+
};
361+
362+
struct Case {
363+
int index; // for debugging
364+
ErrorTags before; // before span finishes
365+
ErrorTags after; // after span finishes
366+
bool error_property_after;
367+
};
368+
369+
// clang-format off
370+
auto test_case = GENERATE(values<Case>({
371+
// Before span finishes After span finishes
372+
// ---------------------------------- ------------------------------------
373+
// error .msg .stack .type error .msg .stack .type error?
374+
// ---------------------------------- --------------------------------------------
375+
// No error tags means no error.
376+
{0, {nullptr, nullptr, nullptr, nullptr}, {nullptr, nullptr, nullptr, nullptr}, false},
377+
// Setting any of the "error.*" tags sets the error property.
378+
{1, {nullptr, "dummy", nullptr, nullptr}, {nullptr, "dummy", nullptr, nullptr}, true},
379+
{2, {nullptr, nullptr, "dummy", nullptr}, {nullptr, nullptr, "dummy", nullptr}, true},
380+
{3, {nullptr, nullptr, nullptr, "dummy"}, {nullptr, nullptr, nullptr, "dummy"}, true},
381+
// Truthy "error" without "error.*" marks as error but sets no tags.
382+
{4, {"true", nullptr, nullptr, nullptr}, {nullptr, nullptr, nullptr, nullptr}, true},
383+
{5, {"TRUE", nullptr, nullptr, nullptr}, {nullptr, nullptr, nullptr, nullptr}, true},
384+
{6, {"1", nullptr, nullptr, nullptr}, {nullptr, nullptr, nullptr, nullptr}, true},
385+
{7, {"true", nullptr, nullptr, nullptr}, {nullptr, nullptr, nullptr, nullptr}, true},
386+
{8, {"True", nullptr, nullptr, nullptr}, {nullptr, nullptr, nullptr, nullptr}, true},
387+
{9, {"T", nullptr, nullptr, nullptr}, {nullptr, nullptr, nullptr, nullptr}, true},
388+
{10, {"t", nullptr, nullptr, nullptr}, {nullptr, nullptr, nullptr, nullptr}, true},
389+
// If "error" is nonempty, not truthy, and not falsy, then set "error.msg" instead.
390+
{11, {"EBADF", nullptr, nullptr, nullptr}, {nullptr, "EBADF", nullptr, nullptr}, true},
391+
{12, {"9", nullptr, nullptr, nullptr}, {nullptr, "9", nullptr, nullptr}, true},
392+
{13, {"oops!", nullptr, nullptr, nullptr}, {nullptr, "oops!", nullptr, nullptr}, true},
393+
{14, {"-0", nullptr, nullptr, nullptr}, {nullptr, "-0", nullptr, nullptr}, true},
394+
// Setting "error.*" unsets "error", but keeps the error property.
395+
{15, {"true", "dummy", nullptr, nullptr}, {nullptr, "dummy", nullptr, nullptr}, true},
396+
{16, {"true", nullptr, "dummy", nullptr}, {nullptr, nullptr, "dummy", nullptr}, true},
397+
{17, {"true", nullptr, nullptr, "dummy"}, {nullptr, nullptr, nullptr, "dummy"}, true},
398+
// If "error" is falsy or empty, then "error" and "error.*" tags are removed, and no error.
399+
{18, {"false", "dummy", "dummy", "dummy"}, {nullptr, nullptr, nullptr, nullptr}, false},
400+
{19, {"FALSE", "dummy", "dummy", "dummy"}, {nullptr, nullptr, nullptr, nullptr}, false},
401+
{20, {"F", "dummy", "dummy", "dummy"}, {nullptr, nullptr, nullptr, nullptr}, false},
402+
{21, {"f", "dummy", "dummy", "dummy"}, {nullptr, nullptr, nullptr, nullptr}, false},
403+
{22, {"0", "dummy", "dummy", "dummy"}, {nullptr, nullptr, nullptr, nullptr}, false},
404+
{23, {"", "dummy", "dummy", "dummy"}, {nullptr, nullptr, nullptr, nullptr}, false},
405+
}));
406+
// clang-format on
407+
408+
CAPTURE(test_case.index);
409+
410+
if (test_case.before.error != nullptr) {
411+
span.SetTag("error", test_case.before.error.get<std::string>());
412+
}
413+
if (test_case.before.msg != nullptr) {
414+
span.SetTag("error.msg", test_case.before.msg.get<std::string>());
415+
}
416+
if (test_case.before.stack != nullptr) {
417+
span.SetTag("error.stack", test_case.before.stack.get<std::string>());
418+
}
419+
if (test_case.before.type != nullptr) {
420+
span.SetTag("error.type", test_case.before.type.get<std::string>());
421+
}
422+
423+
span.FinishWithOptions(finish_options);
424+
const auto& after = *buffer->traces().at(100).finished_spans->at(0);
425+
426+
REQUIRE(after.error == int(test_case.error_property_after));
427+
428+
if (test_case.after.error == nullptr) {
429+
REQUIRE(after.meta.count("error") == 0);
430+
} else {
431+
REQUIRE(after.meta.count("error") == 1);
432+
REQUIRE(after.meta.at("error") == test_case.after.error.get<std::string>());
433+
}
434+
if (test_case.after.msg == nullptr) {
435+
REQUIRE(after.meta.count("error.msg") == 0);
436+
} else {
437+
REQUIRE(after.meta.count("error.msg") == 1);
438+
REQUIRE(after.meta.at("error.msg") == test_case.after.msg.get<std::string>());
439+
}
440+
if (test_case.after.stack == nullptr) {
441+
REQUIRE(after.meta.count("error.stack") == 0);
442+
} else {
443+
REQUIRE(after.meta.count("error.stack") == 1);
444+
REQUIRE(after.meta.at("error.stack") == test_case.after.stack.get<std::string>());
445+
}
446+
if (test_case.after.type == nullptr) {
447+
REQUIRE(after.meta.count("error.type") == 0);
448+
} else {
449+
REQUIRE(after.meta.count("error.type") == 1);
450+
REQUIRE(after.meta.at("error.type") == test_case.after.type.get<std::string>());
451+
}
452+
}
453+
332454
SECTION("operation name can be overridden") {
333455
auto span_id = get_id();
334456
Span span{logger,

0 commit comments

Comments
 (0)
0