Merge pull request #1820 from DataDog/ivoanjo/prof-4535-profiling-use…

…-intake-1.3-format [PROF-4535] Switch profiling to use intake 1.3 format
DataDog · Jan 7, 2022 · 6ee2e72 · 6ee2e72
2 parents f09f897 + 3639187
commit 6ee2e72
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 95 deletions.
diff --git a/benchmarks/profiler_submission.rb b/benchmarks/profiler_submission.rb
@@ -45,7 +45,7 @@ def create_profiler
   end
 
   def check_valid_pprof
-    output_pprof = @adapter_buffer.last[:form]["data[0]"].io
+    output_pprof = @adapter_buffer.last[:form]["data[rubyprofile.pprof]"].io
 
     expected_hashes = [
       "395dd7e65b35be6eede78ac9be072df8d6d79653f8c248691ad9bdd1d8b507de",

diff --git a/lib/ddtrace/ext/profiling.rb b/lib/ddtrace/ext/profiling.rb
@@ -23,13 +23,9 @@ module Transport
         module HTTP
           URI_TEMPLATE_DD_API = 'https://intake.profile.%s/'.freeze
 
-          FORM_FIELD_DATA = 'data[0]'.freeze
-          FORM_FIELD_FORMAT = 'format'.freeze
-          FORM_FIELD_FORMAT_PPROF = 'pprof'.freeze
-          FORM_FIELD_RECORDING_END = 'recording-end'.freeze
-          FORM_FIELD_RECORDING_START = 'recording-start'.freeze
-          FORM_FIELD_RUNTIME = 'runtime'.freeze
-          FORM_FIELD_RUNTIME_ID = 'runtime-id'.freeze
+          FORM_FIELD_RECORDING_START = 'start'.freeze
+          FORM_FIELD_RECORDING_END = 'end'.freeze
+          FORM_FIELD_FAMILY = 'family'.freeze
           FORM_FIELD_TAG_ENV = 'env'.freeze
           FORM_FIELD_TAG_HOST = 'host'.freeze
           FORM_FIELD_TAG_LANGUAGE = 'language'.freeze
@@ -43,13 +39,13 @@ module HTTP
           FORM_FIELD_TAG_SERVICE = 'service'.freeze
           FORM_FIELD_TAG_VERSION = 'version'.freeze
           FORM_FIELD_TAGS = 'tags'.freeze
-          FORM_FIELD_TYPES = 'types[0]'.freeze
-          FORM_FIELD_TYPES_AUTO = 'auto'.freeze
+          FORM_FIELD_INTAKE_VERSION = 'version'.freeze
 
           HEADER_CONTENT_TYPE = 'Content-Type'.freeze
           HEADER_CONTENT_TYPE_OCTET_STREAM = 'application/octet-stream'.freeze
 
-          PPROF_DEFAULT_FILENAME = 'profile.pb.gz'.freeze
+          FORM_FIELD_PPROF_DATA = 'data[rubyprofile.pprof]'.freeze
+          PPROF_DEFAULT_FILENAME = 'rubyprofile.pprof.gz'.freeze
         end
       end
     end

diff --git a/lib/ddtrace/profiling/transport/http/api/endpoint.rb b/lib/ddtrace/profiling/transport/http/api/endpoint.rb
@@ -41,11 +41,10 @@ def call(env, &block)
 
             def build_form(env)
               flush = env.request.parcel.data
-              pprof_file, types = build_pprof(flush)
+              pprof_file = build_pprof(flush)
 
               form = {
-                # NOTE: Redundant w/ 'runtime-id' tag below; may want to remove this later.
-                FORM_FIELD_RUNTIME_ID => flush.runtime_id,
+                FORM_FIELD_INTAKE_VERSION => '3', # Aka 1.3 intake format
                 FORM_FIELD_RECORDING_START => flush.start.utc.iso8601,
                 FORM_FIELD_RECORDING_END => flush.finish.utc.iso8601,
                 FORM_FIELD_TAGS => [
@@ -64,14 +63,10 @@ def build_form(env)
                     .reject { |tag_key| TAGS_TO_IGNORE_IN_TAGS_HASH.include?(tag_key) }
                     .map { |tag_key, tag_value| "#{tag_key}:#{tag_value}" }
                 ],
-                FORM_FIELD_DATA => pprof_file,
-                FORM_FIELD_RUNTIME => flush.language,
-                FORM_FIELD_FORMAT => FORM_FIELD_FORMAT_PPROF
+                FORM_FIELD_PPROF_DATA => pprof_file,
+                FORM_FIELD_FAMILY => flush.language,
               }
 
-              # Add types
-              form[FORM_FIELD_TYPES] = types.join(',')
-
               # Optional fields
               form[FORM_FIELD_TAGS] << "#{FORM_FIELD_TAG_SERVICE}:#{flush.service}" unless flush.service.nil?
               form[FORM_FIELD_TAGS] << "#{FORM_FIELD_TAG_ENV}:#{flush.env}" unless flush.env.nil?
@@ -83,15 +78,13 @@ def build_form(env)
             def build_pprof(flush)
               pprof = encoder.encode(flush)
 
-              # Wrap pprof as a gzipped file
-              gzipped_data = Datadog::Utils::Compression.gzip(pprof.data)
-              pprof_file = Datadog::Vendor::Multipart::Post::UploadIO.new(
-                StringIO.new(gzipped_data),
+              gzipped_pprof_data = Datadog::Utils::Compression.gzip(pprof.data)
+
+              Datadog::Vendor::Multipart::Post::UploadIO.new(
+                StringIO.new(gzipped_pprof_data),
                 HEADER_CONTENT_TYPE_OCTET_STREAM,
                 PPROF_DEFAULT_FILENAME
               )
-
-              [pprof_file, [FORM_FIELD_TYPES_AUTO]]
             end
           end
         end

diff --git a/spec/ddtrace/profiling/transport/http/adapters/net_integration_spec.rb b/spec/ddtrace/profiling/transport/http/adapters/net_integration_spec.rb
@@ -13,7 +13,6 @@
 
 RSpec.describe 'Adapters::Net profiling integration tests' do
   before do
-    skip 'TEST_DATADOG_INTEGRATION is not defined' unless ENV['TEST_DATADOG_INTEGRATION']
     skip 'Profiling is not supported on JRuby' if PlatformHelpers.jruby?
     skip 'Profiling is not supported on TruffleRuby' if PlatformHelpers.truffleruby?
   end
@@ -76,7 +75,6 @@
         allow(Datadog.configuration).to receive(:tags).and_return(tags)
       end
 
-      # rubocop:disable Layout/LineLength
       it 'sends profiles successfully' do
         client.send_profiling_flush(flush)
 
@@ -101,34 +99,31 @@
         body = WEBrick::HTTPUtils.parse_form_data(StringIO.new(request.body), boundary)
 
         expect(body).to include(
-          'runtime-id' => Datadog::Core::Environment::Identity.id,
-          'recording-start' => kind_of(String),
-          'recording-end' => kind_of(String),
-          'data[0]' => kind_of(String),
-          'types[0]' => /auto/,
-          'runtime' => Datadog::Core::Environment::Ext::LANG,
-          'format' => Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_FORMAT_PPROF
+          'version' => '3',
+          'start' => kind_of(String),
+          'end' => kind_of(String),
+          'data[rubyprofile.pprof]' => kind_of(String),
+          'family' => 'ruby',
         )
 
-        tags = body["#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAGS}[]"].list
+        tags = body['tags[]'].list
         expect(tags).to include(
-          /#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_RUNTIME}:#{Datadog::Core::Environment::Ext::LANG}/o,
-          /#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_RUNTIME_ID}:#{uuid_regex.source}/,
-          /#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_RUNTIME_ENGINE}:#{Datadog::Core::Environment::Ext::LANG_ENGINE}/o,
-          /#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_RUNTIME_PLATFORM}:#{Datadog::Core::Environment::Ext::LANG_PLATFORM}/o,
-          /#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_RUNTIME_VERSION}:#{Datadog::Core::Environment::Ext::LANG_VERSION}/o,
-          /#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_PID}:#{Process.pid}/o,
-          /#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_PROFILER_VERSION}:#{Datadog::Core::Environment::Ext::TRACER_VERSION}/o,
-          /#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_LANGUAGE}:#{Datadog::Core::Environment::Ext::LANG}/o,
+          /runtime:ruby/o,
+          /runtime-id:#{uuid_regex.source}/,
+          /runtime_engine:#{Datadog::Core::Environment::Ext::LANG_ENGINE}/o,
+          /runtime_platform:#{Datadog::Core::Environment::Ext::LANG_PLATFORM}/o,
+          /runtime_version:#{Datadog::Core::Environment::Ext::LANG_VERSION}/o,
+          /pid:#{Process.pid}/o,
+          /profiler_version:#{Datadog::Core::Environment::Ext::TRACER_VERSION}/o,
+          /language:ruby/o,
           'test_tag:test_value'
         )
 
         if Datadog::Core::Environment::Container.container_id
           container_id = Datadog::Core::Environment::Container.container_id[0..11]
-          expect(tags).to include(/#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_HOST}:#{container_id}/)
+          expect(tags).to include(/host:#{container_id}/)
         end
       end
-      # rubocop:enable Layout/LineLength
     end
 
     context 'via agent' do

diff --git a/spec/ddtrace/profiling/transport/http/api/endpoint_spec.rb b/spec/ddtrace/profiling/transport/http/api/endpoint_spec.rb
@@ -9,7 +9,6 @@
 require 'ddtrace/profiling/transport/request'
 require 'ddtrace/transport/http/env'
 
-# rubocop:disable Layout/LineLength
 RSpec.describe Datadog::Profiling::Transport::HTTP::API::Endpoint do
   subject(:endpoint) { described_class.new(path, encoder) }
 
@@ -34,9 +33,8 @@
       let(:request) { Datadog::Profiling::Transport::Request.new(flush) }
       let(:flush) { get_test_profiling_flush }
 
-      let(:pprof) { instance_double(Datadog::Profiling::Pprof::Payload, data: data, types: types) }
+      let(:pprof) { instance_double(Datadog::Profiling::Pprof::Payload, data: data) }
       let(:data) { 'pprof_string_data' }
-      let(:types) { [:wall_time_ns] }
       let(:http_response) { instance_double(Datadog::Profiling::Transport::HTTP::Response) }
 
       let(:block) do
@@ -59,22 +57,21 @@
         expect(env.headers).to eq({})
 
         expect(env.form).to include(
-          Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_DATA => kind_of(Datadog::Vendor::Multipart::Post::UploadIO),
-          Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_FORMAT => Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_FORMAT_PPROF,
-          Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_RECORDING_START => flush.start.utc.iso8601,
-          Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_RECORDING_END => flush.finish.utc.iso8601,
-          Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_RUNTIME => flush.language,
-          Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_RUNTIME_ID => flush.runtime_id,
-          Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAGS => array_including(
-            "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_RUNTIME}:#{flush.language}",
-            "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_RUNTIME_ID}:#{flush.runtime_id}",
-            "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_RUNTIME_ENGINE}:#{flush.runtime_engine}",
-            "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_RUNTIME_PLATFORM}:#{flush.runtime_platform}",
-            "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_RUNTIME_VERSION}:#{flush.runtime_version}",
-            "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_PID}:#{Process.pid}",
-            "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_PROFILER_VERSION}:#{flush.profiler_version}",
-            "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_LANGUAGE}:#{flush.language}",
-            "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_HOST}:#{flush.host}"
+          'version' => '3',
+          'data[rubyprofile.pprof]' => kind_of(Datadog::Vendor::Multipart::Post::UploadIO),
+          'start' => flush.start.utc.iso8601,
+          'end' => flush.finish.utc.iso8601,
+          'family' => flush.language,
+          'tags' => array_including(
+            "runtime:#{flush.language}",
+            "runtime-id:#{flush.runtime_id}",
+            "runtime_engine:#{flush.runtime_engine}",
+            "runtime_platform:#{flush.runtime_platform}",
+            "runtime_version:#{flush.runtime_version}",
+            "pid:#{Process.pid}",
+            "profiler_version:#{flush.profiler_version}",
+            "language:#{flush.language}",
+            "host:#{flush.host}"
           )
         )
       end
@@ -95,7 +92,7 @@
         it 'reports the additional tags as part of the tags field' do
           call
 
-          expect(env.form).to include(Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAGS => array_including(
+          expect(env.form).to include('tags' => array_including(
             'test_tag_key:test_tag_value', 'another_tag_key:another_tag_value'
           ))
         end
@@ -117,10 +114,10 @@
         it 'includes service/env/version as tags' do
           call
           expect(env.form).to include(
-            Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAGS => array_including(
-              "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_SERVICE}:#{flush.service}",
-              "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_ENV}:#{flush.env}",
-              "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_VERSION}:#{flush.version}"
+            'tags' => array_including(
+              "service:#{flush.service}",
+              "env:#{flush.env}",
+              "version:#{flush.version}"
             )
           )
         end
@@ -130,7 +127,10 @@
           # the service/env/version in the settings object from the tags if they are available (see settings.rb).
           # But simulating them being different here makes it easier to test that no duplicates are added -- that
           # effectively the tag versions are ignored and we only include the top-level flush versions.
-          let(:tags) { { 'service' => 'service_tag', 'env' => 'env_tag', 'version' => 'version_tag', 'some_other_tag' => 'some_other_value' } }
+          let(:tags) do
+            { 'service' => 'service_tag', 'env' => 'env_tag', 'version' => 'version_tag',
+              'some_other_tag' => 'some_other_value' }
+          end
 
           before do
             flush.tags = tags
@@ -140,43 +140,27 @@
             call
 
             expect(env.form).to include(
-              Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAGS => array_including(
-                "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_SERVICE}:#{flush.service}",
-                "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_ENV}:#{flush.env}",
-                "#{Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAG_VERSION}:#{flush.version}"
+              'tags' => array_including(
+                "service:#{flush.service}",
+                "env:#{flush.env}",
+                "version:#{flush.version}"
               )
             )
           end
 
           it 'does not include the values for these tags from the flush.tags hash' do
             call
 
-            expect(env.form.fetch(Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAGS))
-              .to_not include('service:service_tag', 'env:env_tag', 'version:version_tag')
+            expect(env.form.fetch('tags')).to_not include('service:service_tag', 'env:env_tag', 'version:version_tag')
           end
 
           it 'includes other defined tags' do
             call
 
-            expect(env.form.fetch(Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TAGS))
-              .to include('some_other_tag:some_other_value')
+            expect(env.form.fetch('tags')).to include('some_other_tag:some_other_value')
           end
         end
       end
     end
-
-    context 'when the pprof contains wall & CPU time types' do
-      it_behaves_like 'profile request' do
-        let(:types) { [:wall_time_ns, :cpu_time_ns] }
-
-        it 'includes env tags' do
-          call
-          expect(env.form).to include(
-            Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TYPES => Datadog::Ext::Profiling::Transport::HTTP::FORM_FIELD_TYPES_AUTO
-          )
-        end
-      end
-    end
   end
 end
-# rubocop:enable Layout/LineLength
diff --git a/spec/ddtrace/transport/http/adapters/net_integration_spec.rb b/spec/ddtrace/transport/http/adapters/net_integration_spec.rb
@@ -44,8 +44,13 @@
     end
 
     after do
-      server.shutdown
-      @server_thread.join
+      unless RSpec.current_example.skipped?
+        # When the test is skipped, server has not been initialized and @server_thread would be nil; thus we only
+        # want to touch them when the test actually run, otherwise we would cause the server to start (incorrectly)
+        # and join to be called on a nil @server_thread
+        server.shutdown
+        @server_thread.join
+      end
     end
   end