Log wherever it's needed + cleanup

DataDog · Oct 25, 2023 · 07ced96 · 07ced96
1 parent b2694db
commit 07ced96
Show file tree

Hide file tree

Showing 7 changed files with 45 additions and 92 deletions.
diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Exporter.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Exporter.cpp
@@ -3,8 +3,10 @@
 
 #include "Exporter.h"
 
+#include "EncodedProfile.hpp"
 #include "Exception.h"
 #include "FfiHelper.h"
+#include "Log.h"
 #include "Profile.h"
 #include "Tags.h"
 #include "libdatadog_details/AgentExporter.hpp"
@@ -14,7 +16,6 @@
 #include "libdatadog_details/Tags.hpp"
 #include "libdatadog_details/error_code.hpp"
 #include "libdatadog_helper.hpp"
-#include "EncodedProfile.hpp"
 #include "std_extensions.hpp"
 
 #include <cassert>
@@ -39,7 +40,7 @@ Exporter::ExporterBuilder& Exporter::ExporterBuilder::WithAgent(std::string url)
 
 Exporter::ExporterBuilder& Exporter::ExporterBuilder::WithoutAgent(std::string site, std::string apiKey)
 {
-    assert(_withAgent == false);
+    assert(!_withAgent);
     _agentless = true;
 
     _site = std::move(site);
@@ -71,8 +72,8 @@ std::unique_ptr<libdatadog::detail::AgentExporter> Exporter::ExporterBuilder::Cr
 
     if (result.tag == DDOG_PROF_EXPORTER_NEW_RESULT_ERR)
     {
-        std::unique_ptr<ddog_Error> error(&result.err);
-        throw Exception(std::to_string(error.get()));
+        auto error = detail::make_error(result.err);
+        throw Exception(error.message());
     }
 
     // the AgentExporter instance is acquiring the ownership of the ok ptr
@@ -145,8 +146,11 @@ libdatadog::error_code Exporter::Send(Profile* profile, Tags tags, std::vector<s
 
     if (_fileExporter != nullptr)
     {
-        // link error code ? or log ??
-        _fileExporter->WriteToDisk(ep, profile->GetApplicationName());
+        auto error_code = _fileExporter->WriteToDisk(ep, profile->GetApplicationName());
+        if (!error_code)
+        {
+            Log::Error(error_code.message());
+        }
     }
 
     assert(_exporterImpl != nullptr);

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Exporter.h b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Exporter.h
@@ -64,10 +64,10 @@ class Exporter
         struct AgentEndpoint;
         AgentEndpoint CreateEndpoint();
 
-        bool _agentless;
+        bool _agentless = false;
         std::string _site;
         std::string _apiKey;
-        bool _withAgent;
+        bool _withAgent = false;
         std::string _url;
         //
         std::string _libraryName;

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/LibddprofExporter.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/LibddprofExporter.cpp
@@ -76,7 +76,6 @@ LibddprofExporter::LibddprofExporter(
     IMetadataProvider* metadataProvider,
     IAllocationsRecorder* allocationsRecorder) :
     _sampleTypeDefinitions{std::move(sampleTypeDefinitions)},
-    _locationsAndLinesSize{512},
     _applicationStore{applicationStore},
     _metricsRegistry{metricsRegistry},
     _metadataProvider{metadataProvider},
@@ -89,7 +88,7 @@ LibddprofExporter::LibddprofExporter(
 
 LibddprofExporter::~LibddprofExporter()
 {
-    std::lock_guard lock(_perAppInfoLock);
+    std::lock_guard lck(_perAppInfoLock);
 
     for (auto& [runtimeId, appInfo] : _perAppInfo)
     {
@@ -134,7 +133,7 @@ std::unique_ptr<libdatadog::Exporter> LibddprofExporter::CreateExporter(IConfigu
             exporterBuilder.WithAgent(BuildAgentEndpoint(configuration));
         }
 
-        exporterBuilder.Build();
+        return exporterBuilder.Build();
     }
     catch (libdatadog::Exception const& e)
     {
@@ -266,48 +265,40 @@ std::string LibddprofExporter::GetEnabledProfilersTag(IEnabledProfilers* enabled
 std::string LibddprofExporter::BuildAgentEndpoint(IConfiguration* configuration)
 {
     // handle "with agent" case
-    auto const& url = configuration->GetAgentUrl();
-    if (!url.empty())
-    {
-        _agentUrl = url;
-    }
-    else
+    auto url = configuration->GetAgentUrl(); // copy expected here
+
+    if (url.empty())
     {
         // Agent mode
 
-        std::string agentUrl;
 #if _WINDOWS
         const std::string& namePipeName = configuration->GetNamedPipeName();
         if (!namePipeName.empty())
         {
-            agentUrl = R"(windows:\\.\pipe\)" + namePipeName;
+            url = R"(windows:\\.\pipe\)" + namePipeName;
         }
 #else
         std::error_code ec; // fs::exists might throw if no error_code parameter is provided
         const std::string socketPath = "/var/run/datadog/apm.socket";
         if (fs::exists(socketPath, ec))
         {
-            agentUrl = "unix://" + socketPath;
+            url = "unix://" + socketPath;
         }
 
 #endif
 
-        if (!agentUrl.empty())
-        {
-            _agentUrl = agentUrl;
-        }
-        else
+        if (url.empty())
         {
             // Use default HTTP endpoint
             std::stringstream oss;
             oss << "http://" << configuration->GetAgentHost() << ":" << configuration->GetAgentPort();
-            _agentUrl = oss.str();
+            url = oss.str();
         }
     }
 
-    Log::Info("Using agent endpoint ", _agentUrl);
+    Log::Info("Using agent endpoint ", url);
 
-    return _agentUrl;
+    return url;
 }
 
 LibddprofExporter::ProfileInfoScope LibddprofExporter::GetOrCreateInfo(std::string_view runtimeId)
@@ -327,7 +318,7 @@ void LibddprofExporter::Add(libdatadog::Profile* profile, std::shared_ptr<Sample
         static bool firstTimeError = true;
         if (firstTimeError)
         {
-            Log::Error(success.message());
+            Log::Error("Failed to add a sample: ", success.message());
 
             firstTimeError = false;
         }
@@ -531,8 +522,6 @@ bool LibddprofExporter::Export()
 
         if (_exporter == nullptr)
         {
-            // maybe no need to log error. It was already logged
-            Log::Error("Unable to create exporter for application ", runtimeId);
             return false;
         }
 
@@ -559,20 +548,13 @@ bool LibddprofExporter::Export()
 
         auto filesToSend = std::vector<std::pair<std::string, std::string>>{{MetricsFilename, CreateMetricsFileContent()}};
         std::string json = GetMetadata();
-        try
-        {
-            auto succeeded = _exporter->Send(profile.get(), std::move(additionalTags), std::move(filesToSend), std::move(json));
-            if (!succeeded)
-            {
-                Log::Error(succeeded.message());
-            }
-            exported &= succeeded;
-        }
-        catch (libdatadog::Exception const& e)
+
+        auto error_code = _exporter->Send(profile.get(), std::move(additionalTags), std::move(filesToSend), std::move(json));
+        if (!error_code)
         {
-            Log::Error("Unable to create a request to send the profile: ", e.what());
-            exported = false;
+            Log::Error(error_code.message());
         }
+        exported &= error_code;
     }
 
     return exported;
@@ -610,34 +592,6 @@ std::string LibddprofExporter::GenerateFilePath(const std::string& applicationNa
     return pprofFilePath.string();
 }
 
-// void LibddprofExporter::ExportToDisk(const std::string& applicationName, SerializedProfile const& encodedProfile, int32_t idx)
-//{
-//     auto pprofFilePath = GenerateFilePath(applicationName, idx, ProfileExtension);
-//
-//     std::ofstream file{pprofFilePath, std::ios::out | std::ios::binary};
-//
-//     auto buffer = encodedProfile.GetBuffer();
-//
-//     file.write((char const*)buffer.ptr, buffer.len);
-//     file.close();
-//
-//     if (file.fail())
-//     {
-//         char message[BUFFER_MAX_SIZE];
-//         auto errorCode = errno;
-// #ifdef _WINDOWS
-//         strerror_s(message, BUFFER_MAX_SIZE, errorCode);
-// #else
-//         strerror_r(errorCode, message, BUFFER_MAX_SIZE);
-// #endif
-//         Log::Error("Unable to write profiles on disk: ", pprofFilePath, ". Message (code): ", message, " (", errorCode, ")");
-//     }
-//     else
-//     {
-//         Log::Debug("Profile serialized in ", pprofFilePath);
-//     }
-// }
-
 std::string LibddprofExporter::CreateMetricsFileContent() const
 {
     // prepare metrics to be sent if any

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/LibddprofExporter.h b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/LibddprofExporter.h
@@ -120,11 +120,6 @@ class LibddprofExporter : public IExporter
     std::vector<SampleValueType> _sampleTypeDefinitions;
     fs::path _pprofOutputPath;
 
-    // std::vector<ddog_prof_Location> _locations;
-    // std::vector<ddog_prof_Line> _lines;
-    std::string _agentUrl;
-    std::size_t _locationsAndLinesSize;
-
     // for each application, keep track of a profile, a samples count since the last export and an export count
     std::unordered_map<std::string_view, ProfileInfo> _perAppInfo;
     // ddog_Endpoint _endpoint;

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Profile.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Profile.cpp
@@ -116,18 +116,10 @@ libdatadog::error_code Profile::AddUpscalingRuleProportional(std::vector<std::ui
     auto upscalingRuleAdd = ddog_prof_Profile_add_upscaling_rule_proportional(*_impl, offsets_slice, labelName_slice, groupName_slice, sampled, real);
     if (upscalingRuleAdd.tag == DDOG_PROF_PROFILE_UPSCALING_RULE_ADD_RESULT_ERR)
     {
-        // TODO, TO REWRITE
-        struct ErrorDeleter
-        {
-            void operator()(ddog_Error* o)
-            {
-                ddog_Error_drop(o);
-            }
-        };
-        auto error = std::unique_ptr<ddog_Error, ErrorDeleter>(&upscalingRuleAdd.err);
+        auto error = detail::make_error(upscalingRuleAdd.err);
         std::stringstream ss;
         ss << "(" << groupName << ", " << labelName << ") - [" << std::to_string(sampled) << "/" << std::to_string(real) << "]:"
-           << std::to_string(error.get());
+           << error.message();
         return detail::make_error(ss.str());
     }
 

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/libdatadog_details/AgentExporter.hpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/libdatadog_details/AgentExporter.hpp
@@ -35,7 +35,9 @@ class AgentExporter
             return std::move(ec); // ?? really ?? otherwise it calls the copy constructore :sad:
         }
 
-        // should we use a cancellation token (third parameter) when shutting down takes to much time ?
+        assert(request != nullptr);
+
+        // TODO: should we use a cancellation token (third parameter) when shutting down takes to much time ?
         auto result = ddog_prof_Exporter_send(_exporter.get(), request, nullptr);
 
         if (result.tag == DDOG_PROF_EXPORTER_SEND_RESULT_ERR)
@@ -85,6 +87,7 @@ class AgentExporter
         Request& operator=(Request&& o) noexcept
         {
             _inner = std::exchange(o._inner, nullptr);
+            return *this;
         }
 
         operator ddog_prof_Exporter_Request** ()

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/libdatadog_details/error_code.hpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/libdatadog_details/error_code.hpp
@@ -20,17 +20,20 @@ namespace libdatadog::detail {
 struct ErrorImpl
 {
     ErrorImpl(ddog_Error error) :
-        ErrorImpl(error, std::string())
+        ErrorImpl(error, std::string(), true)
     {
     }
     ErrorImpl(std::string error) :
-        ErrorImpl({}, std::move(error))
+        ErrorImpl({}, std::move(error), false)
     {
     }
 
     ~ErrorImpl()
     {
-        ddog_Error_drop(&_error);
+        if (_freeError)
+        {
+            ddog_Error_drop(&_error);
+        }
     }
 
     ErrorImpl(ErrorImpl const&) = delete;
@@ -47,6 +50,7 @@ struct ErrorImpl
         {
             _error = std::exchange(o._error, {});
             _message.swap(o._message);
+            _freeError = o._freeError;
         }
         return *this;
     }
@@ -61,13 +65,14 @@ struct ErrorImpl
     }
 
 private:
-    ErrorImpl(ddog_Error error, std::string m) :
-        _error{error}, _message{std::move(m)}
+    ErrorImpl(ddog_Error error, std::string m, bool freeError) :
+        _error{error}, _message{std::move(m)}, _freeError{freeError}
     {
     }
 
     ddog_Error _error;
     std::string _message;
+    bool _freeError;
 };
 
 template<class T>