Process review comments by rgacogne and Habbie

Signed-off-by: Otto Moerbeek <otto.moerbeek@open-xchange.com>

diff --git a/pdns/protozero-trace.hh b/pdns/protozero-trace.hh
index ffa73c7c163d7289de4369e8653792ee6b52da1b..aa3b6d6d7921f5b0217814925578217a7076ed8a 100644 (file)
--- a/pdns/protozero-trace.hh
+++ b/pdns/protozero-trace.hh
@@ -37,6 +37,8 @@
 namespace pdns::trace
 {
 
+// https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/common/v1/common.proto
+
 struct AnyValue;
 struct ArrayValue;
 struct KeyValue;
@@ -169,7 +171,8 @@ struct KeyValueList
   }
 };
 
-struct AnyValue : public std::variant<char, std::string, bool, int64_t, double, ArrayValue, KeyValueList, std::vector<uint8_t>>
+using NoValue = char;
+struct AnyValue : public std::variant<NoValue, std::string, bool, int64_t, double, ArrayValue, KeyValueList, std::vector<uint8_t>>
 {
   void encode(protozero::pbf_writer& writer) const;
   static AnyValue decode(protozero::pbf_reader& reader);

diff --git a/pdns/recursordist/docs/performance.rst b/pdns/recursordist/docs/performance.rst
index f5e3cdfa4ca83cc2e30bf4f64028bb8fe50df39e..de7d9fa0eda0c0ae172c3e760f6e2fd317662e14 100644 (file)
--- a/pdns/recursordist/docs/performance.rst
+++ b/pdns/recursordist/docs/performance.rst
@@ -412,11 +412,11 @@ There is no packet cache hit, so SyncRes is called which does a couple of outgoi
 OpenTelemetry Traces
 ^^^^^^^^^^^^^^^^^^^^
 
-OpenTelemetry Traces can be generated by setting :ref:`setting-yaml-recursor.event_trace_enabled` or by using the :doc:`rec_control <manpages/rec_control.1>` subcommand ``set-event-trace-enabled``.
-:program:`Recursor` will set the ``openTelemetryData`` field of ``dnsmessage.proto`` messages crated to contain Protobuf encoded data.
+OpenTelemetry Traces are generated by setting :ref:`setting-yaml-recursor.event_trace_enabled` or by using the :doc:`rec_control <manpages/rec_control.1>` subcommand ``set-event-trace-enabled``.
+:program:`Recursor` will set the ``openTelemetryData`` field of ``dnsmessage.proto`` messages generated to contain OpenTelemetry Traces, encoded as Protobuf data.
 The encoding used is defined in https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/trace/v1/trace.proto.
 
-The example decoder in https://github.com/PowerDNS/pdns/blob/master/contrib/ProtobufLogger.py can show a JSON representation of the data and optionally submit the data (in Protobuf format) to an OpenTelemetry trace collector using a POST to a URL given on the command line.
+The example decoder in https://github.com/PowerDNS/pdns/blob/master/contrib/ProtobufLogger.py displays a JSON representation of the data and optionally submits the data (in Protobuf format) to an OpenTelemetry trace collector using a POST to a URL given on the command line.
 
 An example, encoded in JSON:
 

diff --git a/pdns/recursordist/rec-eventtrace.cc b/pdns/recursordist/rec-eventtrace.cc
index 5c63e946e0abe74ed0b398cad7f5dff2f1566fb9..52c9fe760117373bf144f3e3212e0406556c259e 100644 (file)
--- a/pdns/recursordist/rec-eventtrace.cc
+++ b/pdns/recursordist/rec-eventtrace.cc
@@ -111,8 +111,8 @@ std::vector<pdns::trace::Span> RecEventTrace::convertToOT(const Span& span) cons
     }
     else {
       // It's a close event
-      if (ids.find(event.d_matching) != ids.end()) {
-        auto& work = ret.at(ids.at(event.d_matching));
+      if (const auto match = ids.find(event.d_matching); match != ids.end()) {
+        auto& work = ret.at(match->second);
         addValue(event, work, false);
         work.end_time_unix_nano = static_cast<uint64_t>(event.d_ts + diff);
         spanIDs.emplace_back(work.span_id);

diff --git a/pdns/recursordist/rec-eventtrace.hh b/pdns/recursordist/rec-eventtrace.hh
index d5a8e1e291f0b9214ce1731e14d3afed869f2958..1623b0c2ab990249cb23fa6f0709a38f81ec14b7 100644 (file)
--- a/pdns/recursordist/rec-eventtrace.hh
+++ b/pdns/recursordist/rec-eventtrace.hh
@@ -275,6 +275,9 @@ public:
     return old;
   }
 
+  // The EventScope class is used to close (add an end event) automatically upon the scope object
+  // going out of scope. It is also possible to manually close it, specifying a value to be registered
+  // at the close event. In that case the dt call will become a no-op.
   class EventScope
   {
   public:
@@ -282,12 +285,13 @@ public:
       d_eventTrace(eventTrace),
       d_oldParent(oldParent)
     {
-      if (d_eventTrace.enabled()) {
+      if (d_eventTrace.enabled() && !d_eventTrace.d_events.empty()) {
         d_event = d_eventTrace.d_events.back().d_event;
         d_match = d_eventTrace.d_events.size() - 1;
       }
     }
 
+    // Only int64_t for now needed, might become a template in the future.
     void close(int64_t val)
     {
       if (!d_eventTrace.enabled() || d_closed) {
@@ -300,6 +304,8 @@ public:
 
     ~EventScope()
     {
+      // If the dt is called after an explicit close(), value does not matter.
+      // Otherwise, it signals a implicit close, e.g. an exception was thrown
       close(-1);
     }
     EventScope(const EventScope&) = delete;

diff --git a/pdns/recursordist/test-protozero-trace.cc b/pdns/recursordist/test-protozero-trace.cc
index 2ecf860103d622bf3eba66c2df56cdfa1e3e88c4..68c2561609155f1a93341e6c62e30b38fc2531ee 100644 (file)
--- a/pdns/recursordist/test-protozero-trace.cc
+++ b/pdns/recursordist/test-protozero-trace.cc
@@ -60,7 +60,7 @@ static void testAny(const T& testcase)
 
   protozero::pbf_reader reader{data};
   pdns::trace::AnyValue value = pdns::trace::AnyValue::decode(reader);
-  if (!std::holds_alternative<char>(value)) {
+  if (!std::holds_alternative<pdns::trace::NoValue>(value)) {
     BOOST_CHECK(testcase == std::get<T>(value));
   }
   else {
@@ -116,14 +116,14 @@ BOOST_AUTO_TEST_CASE(traces1)
     .span_id = {0xEE, 0xE1, 0x9B, 0x7E, 0xC3, 0xC1, 0xB1, 0x74},
     .parent_span_id = {0xEE, 0xE1, 0x9B, 0x7E, 0xC3, 0xC1, 0xB1, 0x73},
     .name = "I'm a server span",
+    .kind = pdns::trace::Span::SpanKind::SPAN_KINSERVER,
     .start_time_unix_nano = 1544712660000000000UL,
     .end_time_unix_nano = 1544712661000000000UL,
-    .kind = pdns::trace::Span::SpanKind::SPAN_KINSERVER,
     .attributes = {{"my.span.attr", {"some value"}}}};
   pdns::trace::InstrumentationScope scope = {"my.library", "1.0.0", {{"my.scope.attribute", {"some scope attribute"}}}};
   pdns::trace::ScopeSpans scopespans = {.scope = scope, .spans = {span}};
   pdns::trace::Resource res = {.attributes = {{"service.name", {"my.service"}}}};
-  pdns::trace::ResourceSpans resspans = {{res}, .scope_spans = {scopespans}};
+  pdns::trace::ResourceSpans resspans = {.resource = res, .scope_spans = {scopespans}};
   pdns::trace::TracesData traces = {.resource_spans = {resspans}};
 
   std::string data;