feat(profiling): make Profile upload lock-free (#15302)

## Description

https://datadoghq.atlassian.net/browse/PROF-13044

This PR reduces locking around the libdatadog Profile object to make
uploading non-blocking for sampling. Previously, the Scheduler thread
(which uploads every once in a while) would take the Lock on the Profile
object, preventing any new Samples from being pushed to it until the
upload finishes. Although we don't have any numbers on that, it is
possible that this would trigger adaptive sampling or make us lose some
data.

The new implementation doesn't make uploading completely lock-free, but
already improves it by eliminating the need for locking _during the
upload_. We still need to lock the `Profile` object when we encode it.
It may be possible to optimise this further by having two `Profile`
objects (one we would be writing to, and one that would be _being
uploaded_) but this requires more complicated lock handling to actually
be safe. (Reason is, having two Profiles is still a problem if the
previous encoding/uploading hasn't finished when the next needs to be
encoded/uploaded.)

In the new implementation, the Profile Lock is acquired just before
encoding is done, and released right after. Profiler Stats are also
copied and reset while the Lock is held.

In order to make that work, I had to slightly adapt the interface of
`Uploader` for it to work an `EncodedProfile` instead of a `Profile`,
which allowed me to hoist the encoding outside of the `Uploader` (to the
`UploaderBuilder`), and thus only hold the Profile Lock during
`UploaderBuilder::Build` (as opposed to during the whole of
`Uploader::upload`).
`ProfilerStats` are captured and reset by `std::swap`-ping them with an
empty `ProfilerStats` object, eliminating the previous "singleton-like"
`ProfilerStats` we had. (This works and is trivial because
`ProfilerStats` is a C++ object; we cannot do the same for the `Profile`
itself, as it is managed by the Rust library.)
`Uploader`'s interface has also slightly changed – it is now constructed
with the `ProfilerStats` and `EncodedProfile` directly (not taken as
argument for `upload`).

Author: KowalskiThomas

ddtrace/internal/datadog/profiling/dd_wrapper/include/profile.hpp

@@ -46,8 +46,7 @@ class Profile
     // The profile object is initialized here as a skeleton object, but it
     // cannot be used until it's initialized by libdatadog
     ddog_prof_Profile cur_profile{};
-
-    Datadog::ProfilerStats profiler_stats{};
+    Datadog::ProfilerStats cur_profiler_stats{};
 
     // Internal access methods - not for direct use
     ddog_prof_Profile& profile_borrow_internal();

ddtrace/internal/datadog/profiling/dd_wrapper/include/profiler_stats.hpp

@@ -3,7 +3,6 @@
 #include <cstddef>
 
 #include <string>
-#include <string_view>
 
 namespace Datadog {
 
@@ -17,8 +16,6 @@ a mutex to protect access to the data.
 class ProfilerStats
 {
   private:
-    std::string internal_metadata_json;
-
     // Number of samples collected (one per thread)
     size_t sample_count = 0;
 
@@ -37,9 +34,7 @@ class ProfilerStats
 
     // Returns a JSON string containing relevant Profiler Stats to be included
     // in the libdatadog payload.
-    // The function returned a string_view to a statically allocated string that
-    // is updated every time the function is called.
-    std::string_view get_internal_metadata_json();
+    std::string get_internal_metadata_json();
 
     void reset_state();
 };

ddtrace/internal/datadog/profiling/dd_wrapper/include/uploader.hpp

@@ -22,19 +22,24 @@ class Uploader
     static inline std::atomic<uint64_t> upload_seq{ 0 };
     std::string output_filename;
     ddog_prof_ProfileExporter ddog_exporter{ .inner = nullptr };
+    ddog_prof_EncodedProfile encoded_profile{};
+    Datadog::ProfilerStats profiler_stats;
 
-    bool export_to_file(ddog_prof_EncodedProfile* encoded, std::string_view internal_metadata_json);
+    bool export_to_file(ddog_prof_EncodedProfile& encoded, std::string_view internal_metadata_json);
 
   public:
-    bool upload(ddog_prof_Profile& profile, Datadog::ProfilerStats& profiler_stats);
+    bool upload();
     static void cancel_inflight();
     static void lock();
     static void unlock();
     static void prefork();
     static void postfork_parent();
     static void postfork_child();
 
-    Uploader(std::string_view _url, ddog_prof_ProfileExporter ddog_exporter);
+    Uploader(std::string_view _url,
+             ddog_prof_ProfileExporter ddog_exporter,
+             ddog_prof_EncodedProfile encoded,
+             Datadog::ProfilerStats stats);
     ~Uploader()
     {
         // We need to call _drop() on the exporter and the cancellation token,
@@ -44,6 +49,7 @@ class Uploader
         ddog_CancellationToken_cancel(&cancel);
         ddog_prof_Exporter_drop(&ddog_exporter);
         ddog_CancellationToken_drop(&cancel);
+        ddog_prof_EncodedProfile_drop(&encoded_profile);
     }
 
     // Disable copy constructor and copy assignment operator to avoid double-free
@@ -67,6 +73,9 @@ class Uploader
     {
         ddog_exporter = other.ddog_exporter;
         other.ddog_exporter = { .inner = nullptr };
+        encoded_profile = other.encoded_profile;
+        other.encoded_profile = { .inner = nullptr };
+        profiler_stats = std::move(other.profiler_stats);
         output_filename = std::move(other.output_filename);
         errmsg = std::move(other.errmsg);
     }
@@ -75,8 +84,12 @@ class Uploader
     {
         if (this != &other) {
             ddog_prof_Exporter_drop(&ddog_exporter);
+            ddog_prof_EncodedProfile_drop(&encoded_profile);
             ddog_exporter = other.ddog_exporter;
             other.ddog_exporter = { .inner = nullptr };
+            encoded_profile = other.encoded_profile;
+            other.encoded_profile = { .inner = nullptr };
+            profiler_stats = std::move(other.profiler_stats);
             output_filename = std::move(other.output_filename);
             errmsg = std::move(other.errmsg);
         }

ddtrace/internal/datadog/profiling/dd_wrapper/src/ddup_interface.cpp

@@ -347,6 +347,9 @@ ddup_upload() // cppcheck-suppress unusedFunction
         return false;
     }
 
+    // Build the Uploader, which takes care of serializing the Profile and capturing ProfilerStats.
+    // This takes a reference in a way that locks the areas where the profile might
+    // be modified. It gets cleared and released as soon as serialization is complete (or has failed).
     auto uploader_or_err = Datadog::UploaderBuilder::build();
 
     if (std::holds_alternative<std::string>(uploader_or_err)) {
@@ -359,15 +362,11 @@ ddup_upload() // cppcheck-suppress unusedFunction
 
     // Get the reference to the uploader
     auto& uploader = std::get<Datadog::Uploader>(uploader_or_err);
-    // There are a few things going on here.
-    // * profile_borrow() takes a reference in a way that locks the areas where the profile might
-    //  be modified.  It gets released and cleared after uploading.
-    // * Uploading cancels inflight uploads. There are better ways to do this, but this is what
-    //   we have for now.
-    auto borrowed = Datadog::Sample::profile_borrow();
-    bool success = uploader.upload(borrowed.profile(), borrowed.stats());
-    borrowed.stats().reset_state();
-    return success;
+
+    // Upload without holding any locks (encoding has already been done in UploaderBuilder::build)
+    // This also cancels inflight uploads. There are better ways to do this, but this is what
+    // we have for now.
+    return uploader.upload();
 }
 
 void

ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp

@@ -4,7 +4,6 @@
 #include "profile_borrow.hpp"
 #include "profiler_stats.hpp"
 
-#include <functional>
 #include <iostream>
 
 #include <fcntl.h>
@@ -58,7 +57,7 @@ Datadog::Profile::reset_profile()
         return false;
     }
 
-    profiler_stats.reset_state();
+    cur_profiler_stats.reset_state();
     return true;
 }
 
@@ -189,7 +188,7 @@ Datadog::Profile::one_time_init(SampleType type, unsigned int _max_nframes)
     if (!make_profile(sample_types, &default_period, cur_profile)) {
         if (!already_warned) {
             already_warned = true;
-            std::cerr << "Error initializing profile" << std::endl;
+            std::cerr << "Error initializing cur_profile" << std::endl;
         }
         return;
     }
@@ -228,6 +227,6 @@ Datadog::Profile::postfork_child()
 {
     new (&profile_mtx) std::mutex();
     // Reset the profile to clear any samples collected in the parent process
-    profiler_stats.reset_state();
+    cur_profiler_stats.reset_state();
     reset_profile();
 }

ddtrace/internal/datadog/profiling/dd_wrapper/src/profile_borrow.cpp

@@ -46,5 +46,5 @@ Datadog::ProfileBorrow::profile()
 Datadog::ProfilerStats&
 Datadog::ProfileBorrow::stats()
 {
-    return profile_ptr->profiler_stats;
+    return profile_ptr->cur_profiler_stats;
 }

ddtrace/internal/datadog/profiling/dd_wrapper/src/profiler_stats.cpp

@@ -1,7 +1,6 @@
 #include "profiler_stats.hpp"
 
 #include <charconv>
-#include <string>
 
 namespace {
 
@@ -46,12 +45,13 @@ Datadog::ProfilerStats::reset_state()
     sampling_event_count = 0;
 }
 
-std::string_view
+std::string
 Datadog::ProfilerStats::get_internal_metadata_json()
 {
+    std::string internal_metadata_json;
     internal_metadata_json.reserve(128);
 
-    internal_metadata_json = "{";
+    internal_metadata_json += "{";
 
     internal_metadata_json += R"("sample_count": )";
     append_to_string(internal_metadata_json, sample_count);

ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader.cpp

@@ -13,17 +13,22 @@
 
 using namespace Datadog;
 
-Datadog::Uploader::Uploader(std::string_view _output_filename, ddog_prof_ProfileExporter _ddog_exporter)
+Datadog::Uploader::Uploader(std::string_view _output_filename,
+                            ddog_prof_ProfileExporter _ddog_exporter,
+                            ddog_prof_EncodedProfile _encoded_profile,
+                            Datadog::ProfilerStats _stats)
   : output_filename{ _output_filename }
   , ddog_exporter{ _ddog_exporter }
+  , encoded_profile{ _encoded_profile }
+  , profiler_stats{ std::move(_stats) }
 {
     // Increment the upload sequence number every time we build an uploader.
     // Uploaders are use-once-and-destroy.
     upload_seq++;
 }
 
 bool
-Datadog::Uploader::export_to_file(ddog_prof_EncodedProfile* encoded, std::string_view internal_metadata_json)
+Datadog::Uploader::export_to_file(ddog_prof_EncodedProfile& encoded, std::string_view internal_metadata_json)
 {
     // Write the profile to a file using the following format for filename:
     // <output_filename>.<process_id>.<sequence_number>
@@ -37,7 +42,8 @@ Datadog::Uploader::export_to_file(ddog_prof_EncodedProfile* encoded, std::string
         std::cerr << "Error opening output file " << pprof_filename << ": " << strerror(errno) << std::endl;
         return false;
     }
-    auto bytes_res = ddog_prof_EncodedProfile_bytes(encoded);
+
+    auto bytes_res = ddog_prof_EncodedProfile_bytes(&encoded);
     if (bytes_res.tag == DDOG_PROF_RESULT_BYTE_SLICE_ERR_BYTE_SLICE) {
         std::cerr << "Error getting bytes from encoded profile: "
                   << err_to_msg(&bytes_res.err, "Error getting bytes from encoded profile") << std::endl;
@@ -63,24 +69,10 @@ Datadog::Uploader::export_to_file(ddog_prof_EncodedProfile* encoded, std::string
 }
 
 bool
-Datadog::Uploader::upload(ddog_prof_Profile& profile, Datadog::ProfilerStats& profiler_stats)
+Datadog::Uploader::upload()
 {
-    // Serialize the profile
-    ddog_prof_Profile_SerializeResult serialize_result = ddog_prof_Profile_serialize(&profile, nullptr, nullptr);
-    if (serialize_result.tag !=
-        DDOG_PROF_PROFILE_SERIALIZE_RESULT_OK) { // NOLINT (cppcoreguidelines-pro-type-union-access)
-        auto err = serialize_result.err;         // NOLINT (cppcoreguidelines-pro-type-union-access)
-        errmsg = err_to_msg(&err, "Error serializing pprof");
-        std::cerr << errmsg << std::endl;
-        ddog_Error_drop(&err);
-        return false;
-    }
-    ddog_prof_EncodedProfile* encoded = &serialize_result.ok; // NOLINT (cppcoreguidelines-pro-type-union-access)
-
     if (!output_filename.empty()) {
-        bool ret = export_to_file(encoded, profiler_stats.get_internal_metadata_json());
-        ddog_prof_EncodedProfile_drop(encoded);
-        return ret;
+        return export_to_file(encoded_profile, profiler_stats.get_internal_metadata_json());
     }
 
     std::vector<ddog_prof_Exporter_File> to_compress_files;
@@ -95,11 +87,12 @@ Datadog::Uploader::upload(ddog_prof_Profile& profile, Datadog::ProfilerStats& pr
         });
     }
 
-    auto internal_metadata_json_slice = to_slice(profiler_stats.get_internal_metadata_json());
+    auto internal_metadata_json = profiler_stats.get_internal_metadata_json();
+    auto internal_metadata_json_slice = to_slice(internal_metadata_json);
 
     auto build_res = ddog_prof_Exporter_Request_build(
       &ddog_exporter,
-      encoded,
+      &encoded_profile,
       // files_to_compress_and_export
       {
         .ptr = reinterpret_cast<const ddog_prof_Exporter_File*>(to_compress_files.data()),
@@ -110,7 +103,6 @@ Datadog::Uploader::upload(ddog_prof_Profile& profile, Datadog::ProfilerStats& pr
       &internal_metadata_json_slice,
       nullptr // optional_info_json
     );
-    ddog_prof_EncodedProfile_drop(encoded);
 
     if (build_res.tag ==
         DDOG_PROF_REQUEST_RESULT_ERR_HANDLE_REQUEST) { // NOLINT (cppcoreguidelines-pro-type-union-access)

ddtrace/internal/datadog/profiling/dd_wrapper/src/uploader_builder.cpp

@@ -1,6 +1,7 @@
 #include "uploader_builder.hpp"
 
 #include "libdatadog_helpers.hpp"
+#include "sample.hpp"
 
 #include <mutex>
 #include <numeric>
@@ -186,14 +187,37 @@ Datadog::UploaderBuilder::build()
         return errmsg;
     }
 
+    // Perform profile encoding before creating the Uploader
+    // Also take the Profiler Stats and reset the one being written to
+    ddog_prof_Profile_SerializeResult encoded;
+    Datadog::ProfilerStats stats;
+    {
+        // Only keep the lock for the duration of the encoding operation.
+        auto borrowed = Datadog::Sample::profile_borrow();
+
+        // Swap the ProfilerStats (which replaces the one being written to with an empty state).
+        // We do this first as we still want to reset ProfilerStats if the serialization fails.
+        std::swap(stats, borrowed.stats());
+
+        // Try to encode the Profile (which will also reset it)
+        encoded = ddog_prof_Profile_serialize(&borrowed.profile(), nullptr, nullptr);
+        if (encoded.tag != DDOG_PROF_PROFILE_SERIALIZE_RESULT_OK) {
+            auto err = encoded.err;
+            const std::string errmsg = Datadog::err_to_msg(&err, "Error serializing profile");
+            ddog_Error_drop(&err);
+            ddog_prof_Exporter_drop(ddog_exporter);
+            return errmsg;
+        }
+    }
+
     // We create a std::variant here instead of creating a temporary Uploader object.
-    // i.e. return Datadog::Uploader{ output_filename, *ddog_exporter }
+    // i.e. return Datadog::Uploader{ output_filename, *ddog_exporter, encoded.ok, std::move(stats) }
     // because above code creates a temporary Uploader object, moves it into the
     // variant, and then the destructor of the temporary Uploader object is called
     // when the temporary Uploader object goes out of scope.
     // This was necessary to avoid double-free from calling ddog_prof_Exporter_drop()
     // in the destructor of Uploader. See comments in uploader.hpp for more details.
-    return std::variant<Datadog::Uploader, std::string>{ std::in_place_type<Datadog::Uploader>,
-                                                         output_filename,
-                                                         *ddog_exporter };
+    return std::variant<Datadog::Uploader, std::string>{
+        std::in_place_type<Datadog::Uploader>, output_filename, *ddog_exporter, encoded.ok, std::move(stats)
+    };
 }