[pull] master from PowerDNS:master

pdns/dnsdistdist/dnsdist-actions-factory.cc

@@ -1747,7 +1747,7 @@ class SetTraceAction : public DNSAction
 
   DNSAction::Action operator()([[maybe_unused]] DNSQuestion* dnsquestion, [[maybe_unused]] std::string* ruleresult) const override
   {
-    auto tracer = dnsquestion->ids.getTracer();
+    auto& tracer = dnsquestion->ids.getTracer();
     if (tracer == nullptr) {
       VERBOSESLOG(infolog("SetTraceAction called, but OpenTelemetry tracing is globally disabled. Did you forget to call setOpenTelemetryTracing?"),
                   dnsquestion->getLogger()->info(Logr::Info, "SetTraceAction called, but OpenTelemetry tracing is globally disabled. Did you forget to call setOpenTelemetryTracing?"));

pdns/dnsdistdist/dnsdist-idstate.cc

@@ -117,7 +117,7 @@ std::optional<pdns::trace::dnsdist::Tracer::Closer> InternalQueryState::getClose
   // getTracer returns a Tracer when tracing is globally enabled
   // tracingEnabled tells us whether or not tracing is enabled for this query
   // Should tracing be disabled, *but* we have not processed query rules, we will still return a closer if tracing is globally enabled
-  if (auto tracer = getTracer(); tracer != nullptr && (tracingEnabled || !rulesAppliedToQuery)) {
+  if (auto& tracer = getTracer(); tracer != nullptr && (tracingEnabled || !rulesAppliedToQuery)) {
     ret = d_OTTracer->openSpan(std::string(name), parentSpanID);
   }
 #endif
@@ -128,7 +128,7 @@ std::optional<pdns::trace::dnsdist::Tracer::Closer> InternalQueryState::getClose
 {
   std::optional<pdns::trace::dnsdist::Tracer::Closer> ret{std::nullopt};
 #ifndef DISABLE_PROTOBUF
-  if (auto tracer = getTracer(); tracer != nullptr) {
+  if (auto& tracer = getTracer(); tracer != nullptr) {
     auto parentSpanID = d_OTTracer->getLastSpanIDForName(std::string(parentSpanName));
     ret = getCloser(name, parentSpanID);
   }
@@ -140,7 +140,7 @@ std::optional<pdns::trace::dnsdist::Tracer::Closer> InternalQueryState::getClose
 {
   std::optional<pdns::trace::dnsdist::Tracer::Closer> ret{std::nullopt};
 #ifndef DISABLE_PROTOBUF
-  if (auto tracer = getTracer(); tracer != nullptr) {
+  if (auto& tracer = getTracer(); tracer != nullptr) {
     ret = getCloser(std::string(name), tracer->getLastSpanID());
   }
 #endif
@@ -155,7 +155,7 @@ std::optional<pdns::trace::dnsdist::Tracer::Closer> InternalQueryState::getRules
   // getTracer returns a Tracer when tracing is globally enabled
   // tracingEnabled tells us whether or not tracing is enabled for this query
   // Should tracing be disabled, *but* we have not processed query rules, we will still return a closer if tracing is globally enabled
-  if (auto tracer = getTracer(); tracer != nullptr && (tracingEnabled || !rulesAppliedToQuery)) {
+  if (auto& tracer = getTracer(); tracer != nullptr && (tracingEnabled || !rulesAppliedToQuery)) {
     auto parentSpanID = tracer->getLastSpanID();
     auto name = ruleType + prefix + std::string(ruleName);
     ret = tracer->openSpan(name, parentSpanID);

pdns/dnsdistdist/dnsdist-idstate.hh

@@ -128,20 +128,23 @@ struct InternalQueryState
    *
    * @return
    */
-  std::shared_ptr<pdns::trace::dnsdist::Tracer> getTracer()
+  std::shared_ptr<pdns::trace::dnsdist::Tracer>& getTracer()
   {
 #ifdef DISABLE_PROTOBUF
-    return nullptr;
+    return d_OTTracer;
 #else
+    if ((d_OTTracingEnabledInConfiguration && !*d_OTTracingEnabledInConfiguration) || d_OTTracer != nullptr) {
+      return d_OTTracer;
+    }
     if (dnsdist::configuration::getCurrentRuntimeConfiguration().d_openTelemetryTracing) {
-      if (d_OTTracer != nullptr) {
-        return d_OTTracer;
-      }
       // OpenTelemetry tracing is enabled, but we don't have a tracer yet
+      d_OTTracingEnabledInConfiguration = true;
       d_OTTracer = pdns::trace::dnsdist::Tracer::getTracer();
-      return d_OTTracer;
     }
-    return nullptr;
+    else {
+      d_OTTracingEnabledInConfiguration = false;
+    }
+    return d_OTTracer;
 #endif
   }
 
@@ -231,6 +234,9 @@ public:
   uint16_t udpPayloadSize{0}; // Max UDP payload size from the query // 2
   uint16_t sendTraceParentToDownstreamID{0}; // Whether or not to add a TRACEPARENT EDNS option to downstreams, set to non-0 for the EDNS Option ID
   std::optional<bool> dnssecOK;
+#ifndef DISABLE_PROTOBUF
+  std::optional<bool> d_OTTracingEnabledInConfiguration; // Whether OpenTelemetry tracing is enabled in the configuration. This prevents having to check the configuration several times for the same query
+#endif /* DISABLE_PROTOBUF */
   dnsdist::Protocol protocol; // 1
   uint8_t restartCount{0}; // 1
   bool ednsAdded{false};

pdns/dnsdistdist/dnsdist-lua-bindings-dnsquestion.cc

@@ -374,7 +374,7 @@ void setupLuaBindingsDNSQuestion([[maybe_unused]] LuaContext& luaCtx)
 #ifdef DISABLE_PROTOBUF
       return std::nullopt;
 #else
-      if (auto tracer = dnsQuestion.ids.getTracer(); tracer != nullptr && dnsQuestion.ids.tracingEnabled) {
+      if (auto& tracer = dnsQuestion.ids.getTracer(); tracer != nullptr && dnsQuestion.ids.tracingEnabled) {
         auto traceID = tracer->getTraceID();
         return std::string(traceID.begin(), traceID.end());
       }
@@ -388,7 +388,7 @@ void setupLuaBindingsDNSQuestion([[maybe_unused]] LuaContext& luaCtx)
 #ifdef DISABLE_PROTOBUF
       return std::nullopt;
 #else
-      if (auto tracer = dnsQuestion.ids.getTracer(); tracer != nullptr && dnsQuestion.ids.tracingEnabled) {
+      if (auto& tracer = dnsQuestion.ids.getTracer(); tracer != nullptr && dnsQuestion.ids.tracingEnabled) {
         auto spanID = tracer->getLastSpanID();
         return std::string(spanID.begin(), spanID.end());
       }

pdns/dnsdistdist/dnsdist-opentelemetry.cc

@@ -219,7 +219,7 @@ SpanID Tracer::getRootSpanID()
     return data->d_oldAndNewRootSpanID.newID;
   }
 
-  if (auto& spans = data->d_spans; !spans.empty()) {
+  if (const auto& spans = data->d_spans; !spans.empty()) {
     auto iter = std::find_if(spans.cbegin(), spans.cend(), [](const auto& span) { return span.parent_span_id == pdns::trace::s_emptySpanID; });
     if (iter != spans.cend()) {
       return iter->span_id;
@@ -251,7 +251,7 @@ SpanID Tracer::getLastSpanIDForName([[maybe_unused]] const std::string& name)
   return 0;
 #else
   auto data = d_data.read_only_lock();
-  if (auto& spans = data->d_spans; !spans.empty()) {
+  if (const auto& spans = data->d_spans; !spans.empty()) {
     if (auto iter = std::find_if(spans.rbegin(),
                                  spans.rend(),
                                  [name](const miniSpan& span) { return span.name == name; });
@@ -320,11 +320,11 @@ void Tracer::Closer::setAttribute([[maybe_unused]] const std::string& key, [[may
 #ifdef DISABLE_PROTOBUF
   return;
 #else
-  return d_tracer->setSpanAttribute(d_spanID, key, value);
+  d_tracer->setSpanAttribute(d_spanID, key, value);
 #endif
 }
 
-std::vector<uint8_t> makeEDNSTraceParentOption(std::shared_ptr<Tracer> tracer)
+std::vector<uint8_t> makeEDNSTraceParentOption([[maybe_unused]] const std::shared_ptr<Tracer>& tracer)
 {
   std::vector<uint8_t> ret;
 #ifndef DISABLE_PROTOBUF
@@ -343,7 +343,7 @@ std::vector<uint8_t> makeEDNSTraceParentOption(std::shared_ptr<Tracer> tracer)
   return ret;
 }
 
-bool addTraceparentEdnsOptionToPacketBuffer(PacketBuffer& origBuf, const std::shared_ptr<Tracer>& tracer, const size_t qnameWireLength, const size_t proxyProtocolPayloadSize, const uint16_t traceparentOptionCode, const bool isTCP)
+bool addTraceparentEdnsOptionToPacketBuffer([[maybe_unused]] PacketBuffer& origBuf, [[maybe_unused]] const std::shared_ptr<Tracer>& tracer, [[maybe_unused]] const size_t qnameWireLength, [[maybe_unused]] const size_t proxyProtocolPayloadSize, [[maybe_unused]] const uint16_t traceparentOptionCode, [[maybe_unused]] const bool isTCP)
 {
 #ifndef DISABLE_PROTOBUF
   if (tracer == nullptr) {
@@ -352,19 +352,20 @@ bool addTraceparentEdnsOptionToPacketBuffer(PacketBuffer& origBuf, const std::sh
   // buf contains the whole DNS query without PROXY protocol and TCP length header
   PacketBuffer buf{origBuf.begin() + proxyProtocolPayloadSize + (isTCP ? 2 : 0), origBuf.end()};
 
-  uint16_t optRDPosition;
-  size_t remaining;
+  uint16_t optRDPosition{};
+  size_t remaining{};
   bool queryHadEdns = ::dnsdist::getEDNSOptionsStart(buf, qnameWireLength, &optRDPosition, &remaining) == 0;
   if (queryHadEdns) {
     size_t optLen = buf.size() - optRDPosition - remaining;
+    // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
     removeEDNSOptionFromOPT(reinterpret_cast<char*>(buf.data() + optRDPosition), &optLen, traceparentOptionCode);
   }
 
   auto opt = pdns::trace::dnsdist::makeEDNSTraceParentOption(tracer);
   bool ednsAdded{false};
   bool optionAdded{false};
-  uint16_t maxEdnsSize = queryHadEdns ? (uint16_t)(buf.at(optRDPosition - 6) << 8) + buf.at(optRDPosition - 5) : 512;
-  setEDNSOption(buf, traceparentOptionCode, std::string(opt.begin(), opt.end()), isTCP ? std::numeric_limits<uint16_t>().max() : maxEdnsSize, ednsAdded, optionAdded);
+  uint16_t maxEdnsSize = queryHadEdns ? static_cast<uint16_t>(buf.at(optRDPosition - 6) << 8) + buf.at(optRDPosition - 5) : 512;
+  setEDNSOption(buf, traceparentOptionCode, std::string(opt.begin(), opt.end()), isTCP ? std::numeric_limits<uint16_t>::max() : maxEdnsSize, ednsAdded, optionAdded);
 
   if (isTCP) {
     const std::array<uint8_t, 2> sizeBytes{static_cast<uint8_t>(buf.size() / 256), static_cast<uint8_t>(buf.size() % 256)};

pdns/dnsdistdist/dnsdist-opentelemetry.hh

@@ -217,7 +217,7 @@ public:
     }
     Closer(const Closer&) = delete;
     Closer& operator=(const Closer&) = delete;
-    Closer& operator=(Closer&& rhs) noexcept
+    Closer& operator=([[maybe_unused]] Closer&& rhs) noexcept
     {
 #ifndef DISABLE_PROTOBUF
       this->d_tracer = std::move(rhs.d_tracer);
@@ -228,7 +228,7 @@ public:
 #endif
       return *this;
     }
-    Closer(Closer&& rhs)
+    Closer([[maybe_unused]] Closer&& rhs) noexcept
     {
 #ifndef DISABLE_PROTOBUF
       this->d_tracer = std::move(rhs.d_tracer);
@@ -362,6 +362,6 @@ private:
 #endif
 };
 
-std::vector<uint8_t> makeEDNSTraceParentOption(std::shared_ptr<Tracer> tracer);
-bool addTraceparentEdnsOptionToPacketBuffer(PacketBuffer& origBuf, const std::shared_ptr<Tracer>& tracer, const size_t qnameWireLength, const size_t proxyProtocolPayloadSize, const uint16_t traceparentOptionCode = EDNSOptionCode::TRACEPARENT, const bool isTCP = false);
+std::vector<uint8_t> makeEDNSTraceParentOption(const std::shared_ptr<Tracer>& tracer);
+bool addTraceparentEdnsOptionToPacketBuffer(PacketBuffer& origBuf, const std::shared_ptr<Tracer>& tracer, size_t qnameWireLength, size_t proxyProtocolPayloadSize, uint16_t traceparentOptionCode = EDNSOptionCode::TRACEPARENT, bool isTCP = false);
 } // namespace pdns::trace::dnsdist

pdns/dnsdistdist/dnsdist-protobuf.cc

@@ -254,7 +254,7 @@ void DNSDistProtoBufMessage::serialize(std::string& data, bool withOpenTelemetry
     }
   }
 
-  if (auto tracer = d_dq.ids.getTracer(); tracer != nullptr && d_dq.ids.tracingEnabled) {
+  if (auto& tracer = d_dq.ids.getTracer(); tracer != nullptr && d_dq.ids.tracingEnabled) {
     msg.setOpenTelemetryTraceID(tracer->getTraceID());
     if (withOpenTelemetryTraceData) {
       msg.setOpenTelemetryData(tracer->getOTProtobuf());

pdns/dnsdistdist/dnsdist-tcp-downstream.cc

@@ -289,7 +289,7 @@ IOState TCPConnectionToBackend::sendQuery(std::shared_ptr<TCPConnectionToBackend
   DEBUGLOG("sending query to backend " << conn->getDS()->getNameWithAddr() << " over FD " << conn->d_handler->getDescriptor());
 
 #ifndef DISABLE_PROTOBUF
-  if (auto tracer = conn->d_currentQuery.d_query.d_idstate.getTracer(); conn->d_currentQuery.d_query.d_idstate.sendTraceParentToDownstreamID != 0 && tracer != nullptr) {
+  if (auto& tracer = conn->d_currentQuery.d_query.d_idstate.getTracer(); conn->d_currentQuery.d_query.d_idstate.sendTraceParentToDownstreamID != 0 && tracer != nullptr) {
     auto ednsAdded = pdns::trace::dnsdist::addTraceparentEdnsOptionToPacketBuffer(
       conn->d_currentQuery.d_query.d_buffer,
       tracer,

pdns/dnsdistdist/dnsdist.cc

@@ -1899,7 +1899,7 @@ bool assignOutgoingUDPQueryToBackend(std::shared_ptr<DownstreamState>& downstrea
                 dnsQuestion.getLogger()->info(Logr::Info, "Relayed query to backend", "backend.name", Logging::Loggable(downstream->getName()), "backend.address", Logging::Loggable(downstream->d_config.remote), "dnsdist.xsk", Logging::Loggable(!actuallySend)));
 
 #ifndef DISABLE_PROTOBUF
-    if (auto tracer = dnsQuestion.ids.getTracer(); dnsQuestion.ids.sendTraceParentToDownstreamID != 0 && tracer != nullptr) {
+    if (auto& tracer = dnsQuestion.ids.getTracer(); dnsQuestion.ids.sendTraceParentToDownstreamID != 0 && tracer != nullptr) {
       auto ednsAdded = pdns::trace::dnsdist::addTraceparentEdnsOptionToPacketBuffer(
         dnsQuestion.getMutableData(),
         tracer,

regression-tests.dnsdist/test_OpenTelemetryTracing.py

@@ -14,6 +14,7 @@
 import google.protobuf.json_format
 import opentelemetry.proto.trace.v1.trace_pb2
 
+from dnsdisttests import pickAvailablePort
 import test_Protobuf
 
 
@@ -66,12 +67,8 @@ def sendQueryAndGetProtobuf(
             self.assertTrue(receivedResponse)
             self.assertEqual(response, receivedResponse)
 
-        if self._protobufQueue.empty():
-            # let the protobuf messages the time to get there
-            time.sleep(1)
-
         # check the protobuf message corresponding to the UDP query
-        return self.getFirstProtobufMessage()
+        return self.getFirstProtobufMessage(timeout=1)
 
     def checkOTData(
         self,
@@ -719,6 +716,11 @@ def servfailOnTraceParent(request: dns.message.Message):
 
 
 class TestOpenTelemetryTracingStripIncomingTraceParent(DNSDistOpenTelemetryProtobufTest):
+    # this test suite uses a different responder port
+    # because it uses a different responder logic so we
+    # need to make sure we are not hitting the backend
+    # from a different test
+    _testServerPort = pickAvailablePort()
     _yaml_config_params = [
         "_testServerPort",
     ]
@@ -821,6 +823,11 @@ def verifyTraceparentInQuery(request: dns.message.Message):
 
 
 class TestOpenTelemetryTracingSendTraceparentDownstream(DNSDistOpenTelemetryProtobufTest):
+    # this test suite uses a different responder port
+    # because it uses a different responder logic so we
+    # need to make sure we are not hitting the backend
+    # from a different test
+    _testServerPort = pickAvailablePort()
     _yaml_config_params = [
         "_testServerPort",
     ]