Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PROF-4535] Switch profiling to use intake 1.3 format #1820

Merged
merged 5 commits into from
Jan 7, 2022
Merged

[PROF-4535] Switch profiling to use intake 1.3 format #1820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bc93298
Remove test for unused behavior
ivoanjo Jan 5, 2022
d3c40da
Enable profiling integration tests without TEST_DATADOG_INTEGRATION
ivoanjo Jan 5, 2022
fb72f4b
Avoid referencing payload constants in tests
ivoanjo Jan 5, 2022
b6cd08e
[PROF-4535] Switch profiling to use intake 1.3 format
ivoanjo Jan 5, 2022
3639187
Fix starting test server when test was skipped
ivoanjo Jan 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmarks/profiler_submission.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,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",
Expand Down
16 changes: 6 additions & 10 deletions lib/ddtrace/ext/profiling.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines -26 to -32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, we don't need to support version 1.2 anymore at all?

I ask because, for tracing when we introduce a new transport version, we normally keep the old one as the user's Datadog agent might not be up to date with the latest transport version yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great question :)

We don't need to support the old version at all because for profiling, there's no format-specific logic in the agent. The agent just acts as a rather dumb proxy -- receives stuff from the library and then repeats it back to the profiling backend.

So we are free to make these changes.

Also, there aren't yet any plans to sunset the old format, so older versions of the profiler will continue to work (but we always recommend being on the latest!).

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
Expand All @@ -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
Expand Down
23 changes: 8 additions & 15 deletions lib/ddtrace/profiling/transport/http/api/endpoint.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 => [
Expand All @@ -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?
Expand All @@ -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
Expand Down
35 changes: 15 additions & 20 deletions spec/ddtrace/profiling/transport/http/adapters/net_integration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand All @@ -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
Expand Down
78 changes: 31 additions & 47 deletions spec/ddtrace/profiling/transport/http/api/endpoint_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
9 changes: 7 additions & 2 deletions spec/ddtrace/transport/http/adapters/net_integration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down