Namespacing: Profiling

[marcotc]

This PR moves the profiler from ddtrace/profiling to datadog/profiling.

The executable ddtracerb was left with its original name, as it today represents an executable for the whole ddtrace gem, despite being used mostly for profiling.

The ddtrace_profiling_native_extension directory was not renamed: this is a more involved change than a simple renaming, and can be done later as this change will have no user impact.

Breaking changes

• Changed: Profiler files moved from ddtrace/profiling to datadog/profiling.

To do after merge

• Merge documentation PR: Update profiling namespacing for ddtrace 1.0 documentation#12912

[delner]

My feeling is that:

1. ddtracerb --> ddprofrb
2. Datadog::Tasks --> Datadog::Profiling::Tasks
3. require 'ddtrace/profiling/preload' --> require 'datadog/profiling/preload'

Thoughts @ivoanjo ?

[ivoanjo]

Looks reasonable. Some extra notes:

1. I saw that ./ddtrace/ext/profiling.rb has not been removed, that looks like an oversight.
2. Inside the ext folder, we also have the ddtrace_profiling_native_extension folder. We probably want to rename that as well; I'd be happy to send a follow-up PR with it (but we can do it at any time, since that name is not a public api)
3. CI seems to be complaining about a lot of unrelated things; I guess this was branched off of an unstable branch.

---

Update onboarding documentation with path renaming: https://github.com/DataDog/documentation/blame/eee3812e9a1620455995422bd50b83779b21d537/content/en/tracing/profiler/enabling/ruby.md#L92. Should we keep an alias to ddtrace/profiling/preload for now?

Since we're doing this much cleanup for 1.0, I don't think it quite makes sense to hold on to ddtrace/profiling/preload as an alias.

BUT, I don't like that customers may run into this error and then have figure out on their own what's up:

[1] pry(main)> require 'ddtrace/profiling/preload'
LoadError: cannot load such file -- ddtrace/profiling/preload

So, rather than an alias, I would suggest leaving the following inside ddtrace/profiling/preload.rb:

raise LoadError, "this file has been moved. Please update your require to load `datadog/profiling/preload` instead of `ddtrace/profiling/preload`"

---

Re:

1. ddtracerb --> ddprofrb

I would like to keep the current name, for consistency.

• dd-trace-py has a similar tool as well
• ddtracerb comes from the name of the gem vs it being related to tracing

Once we decide to rename the gem, I think it makes sense to deprecate and rename this.

2. Datadog::Tasks --> Datadog::Profiling::Tasks

No special feelings, either one seems reasonable.

[delner]

Overall I agree with all of @ivoanjo 's feedback.

Regarding ddtracerb:

I would like to keep the current name, for consistency.

This is fine. I think we're aligned.

[codecov-commenter]

Codecov Report

Merging #1849 (e979f1d) into 1.0 (3c4f2dd) will increase coverage by 0.06%.
The diff coverage is 99.01%.

@@            Coverage Diff             @@
##              1.0    #1849      +/-   ##
==========================================
+ Coverage   98.16%   98.22%   +0.06%     
==========================================
  Files         957      959       +2     
  Lines       47030    47052      +22     
==========================================
+ Hits        46168    46219      +51     
+ Misses        862      833      -29     

Impacted Files	Coverage Δ
lib/datadog/ci.rb	69.23% <0.00%> (ø)
lib/datadog/profiling/backtrace_location.rb	100.00% <ø> (ø)
lib/datadog/profiling/buffer.rb	100.00% <ø> (ø)
...ib/datadog/profiling/collectors/code_provenance.rb	100.00% <ø> (ø)
lib/datadog/profiling/event.rb	100.00% <ø> (ø)
lib/datadog/profiling/ext/forking.rb	100.00% <ø> (ø)
lib/datadog/profiling/flush.rb	100.00% <ø> (ø)
lib/datadog/profiling/native_extension.rb	83.33% <ø> (ø)
lib/datadog/profiling/pprof/message_set.rb	100.00% <ø> (ø)
lib/datadog/profiling/pprof/payload.rb	100.00% <ø> (ø)
... and 171 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3bc033...e979f1d. Read the comment docs.

[delner]

Seems straightforward to me. I think it's reasonable to merge as-is, if necessary.

Only inquiries I have are:

1. Should Datadog::Profiler become Datadog::Profiling::Profiler? It's wordy but respects namespaces? How it is now would be fine, just exploring. Whatever we do here, Datadog::Tracer should match.
2. Should we hide Datadog.profiler? Do users actually directly interact with Datadog.profiler? I think we should consider this if they don't.

[ivoanjo]

LGTM 👍

2. Should we hide Datadog.profiler? Do users actually directly interact with Datadog.profiler? I think we should consider this if they don't.

Yeap, sounds good.

As discussed via chat, I think something like

module Datadog
  module Profiling
    def self.start_if_enabled
      # Getting the profiler instance triggers start as a side-effect;
      # otherwise we get nil
      !!Datadog.send(:components).profiler
    end
  end
end

would be more than enough :)

[marcotc]

I've made the changes as suggested. I'm having a hard time with the integration apps failures, but getting there.

[ivoanjo]

Totally missed this weird bit, but with the fix it LGTM 👍

[ivoanjo]

YAY FOR INTEGRATION TESTS :)

Oops I totally lied to you. 🤦‍♂️ See if you can spot what I totally forgot AND that the tests were catching because the webserver DID fork.

Suggested change

	def self.start_if_enabled
	# Getting the profiler instance triggers start as a side-effect;
	# otherwise we get nil
	!!Datadog.send(:components).profiler
	end
	def self.start_if_enabled
	# If the profiler was not previously touched, getting the profiler instance triggers start as a side-effect
	# otherwise we get nil
	profiler = Datadog.send(:components).profiler
	# ...but we still try to start it BECAUSE if the process forks, the profiler will exist but may
	# not yet have been started in the fork
	profiler.start if profiler
	!!profiler
	end