ekump/decouple core transport #3150

ekump · 2023-09-21T15:58:14Z

What does this PR do?
Properly namespace the various transport modules to aid in decoupling Tracing from Core. There should be no functional changes.

Datadog::Transport (ddtrace/transport) was primarily used by Tracing so it has been moved to Datadog::Tracing::Transport. The exception to this is the ext file which is a public API. We should discuss moving this in 2.0 (should be relatively easy).

Datadog::Core::Transport was only used by Datadog::Core::Remote. It has been moved to Datadog::Core::Remote::Transport.

Files common to both Remote and Tracing were moved to Datadog::Core::Transport.

Motivation:
Decouple Tracing from Core

Additional Notes:
The scope of this PR is primarily decoupling. There is probably a fair amount of work that can be done to simplify the transport code in general, but that can be done separately.

How to test the change?

For Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles
credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

…port

…ansport::parcel to core

…trace folder

marcotc · 2023-09-22T20:51:58Z

.rubocop_todo.yml

@@ -32,12 +32,6 @@ Lint/MissingSuper:
    - 'lib/datadog/profiling/pprof/converter.rb'


Nice cleanup!

marcotc

Awesome work, @ekump!

anmarchenko

nice, I will migrate to use Net::HTTP adapter from Core for agentless mode as soon as it gets merged

GustavoCaso · 2023-09-25T09:41:40Z

lib/datadog/core/remote/transport/http.rb

+# Below should be:
+# require_relative '../../transport/http/api'


Is this comment relevant?

GustavoCaso · 2023-09-25T09:41:55Z

lib/datadog/core/remote/transport/http.rb

+# Below should be:
+# require_relative '../../transport/http/builder'


Is this comment relevant?

GustavoCaso · 2023-09-25T09:43:10Z

lib/datadog/core/remote/transport/http/api.rb

+# Below should be:
+# require_relative '../../../../ddtrace/transport/http/api/spec'


Is this comment relevant? And any other comment that state what the actual require should look like 😄

These comments were already in place. I just updated them to reflect this PR's changes. If we think they're superfluous, I'm happy to remove them.

GustavoCaso · 2023-09-25T09:44:43Z

Amazing work @ekump 🎉

I wondered if you explored avoiding the duplicated portions of core/transport and tracing/transport and trying to work with what we currently have to not have the code duplicated? Or if we plan on working on subsequent PR to have a more flexible core transport that remote, tracing, and CI can use?

ekump · 2023-09-25T14:18:18Z

Amazing work @ekump 🎉

I wondered if you explored avoiding the duplicated portions of core/transport and tracing/transport and trying to work with what we currently have to not have the code duplicated? Or if we plan on working on subsequent PR to have a more flexible core transport that remote, tracing, and CI can use?

@GustavoCaso - I think there is a fair amount of interest from everyone to a) simplify transport and b) do what we can to reduce the duplicate code between remote and tracing. But, the scope of this PR was just decoupling. I'm hoping we'll be able to revisit the transport code sooner, rather than later to give it more attention.

github-actions bot added core Involves Datadog core libraries integrations Involves tracing integrations tracing labels Sep 21, 2023

ekump force-pushed the ekump/decouple-core-transport branch 4 times, most recently from 121d9c1 to 1314404 Compare September 21, 2023 20:14

github-actions bot added the profiling Involves Datadog profiling label Sep 22, 2023

ekump force-pushed the ekump/decouple-core-transport branch from c3f5bb3 to 7a186e3 Compare September 22, 2023 18:39

ekump added 21 commits September 22, 2023 16:32

WIP: Moving transport traces to tracing

406c01b

move ddtrace/transport to tracing and update file references

c6d2028

namespace Datadog::Transport from ddtrace to DDatadog::Tracing::Trans…

84ce9f3

…port

move Core::Transport to Core::Remote::Transport

90c280d

move Transport::HTTP::API::Map and Fallback to core

918cfc2

move tracing::transport::http::env to core

2cb1a15

move tracing::transport::request to core

5754c16

move tracing::transport::http::response to core

35fea19

move tracing::transport::http::adapters to core

bdb8945

move datadog::tracing::transport::http::api::endpoint and tracing::tr…

c2cf885

…ansport::parcel to core

move Tracing::Transport::Response to core

a00f91f

fixing contrib tests for transport refactor

667cfb3

fix bad merge for faux_writer

68c0e8a

fix rubocop errors

96a411f

cleanup TODOs for transport refactor

e9d47d2

creating Datadog::Core::Transport::Ext

32d7c6c

cleaning up contrib test failures

02b6c42

export Datadog::Core::Transport::Ext to Datadog::Transport::Ext in dd…

a4c0001

…trace folder

cleaning up rubocop errors

ffc5cb8

move rbs files to correct folders for transport refactor

4ca9b95

update namespace for decoupling transport

01e2fac

forgot to properly namespace Datadog::Core::Transport::HTTP::Env

caecc2c

ekump force-pushed the ekump/decouple-core-transport branch from 7a186e3 to caecc2c Compare September 22, 2023 20:32

ekump marked this pull request as ready for review September 22, 2023 20:33

ekump requested a review from a team September 22, 2023 20:33

marcotc reviewed Sep 22, 2023

View reviewed changes

.rubocop_todo.yml

@@ -32,12 +32,6 @@ Lint/MissingSuper:

- 'lib/datadog/profiling/pprof/converter.rb'

Copy link

Member

marcotc Sep 22, 2023

Choose a reason for hiding this comment

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

Nice cleanup!

marcotc approved these changes Sep 22, 2023

View reviewed changes

anmarchenko approved these changes Sep 25, 2023

View reviewed changes

GustavoCaso reviewed Sep 25, 2023

View reviewed changes

ekump merged commit 9810fe0 into master Sep 25, 2023
217 checks passed

ekump deleted the ekump/decouple-core-transport branch September 25, 2023 19:42

github-actions bot added this to the 1.15.0 milestone Sep 25, 2023

duncanista mentioned this pull request Oct 11, 2023

fix: latest version of dd-trace broke the gem DataDog/datadog-lambda-rb#85

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ekump/decouple core transport #3150

ekump/decouple core transport #3150

ekump commented Sep 21, 2023 •

edited

Loading

marcotc Sep 22, 2023

marcotc left a comment

anmarchenko left a comment

GustavoCaso Sep 25, 2023

GustavoCaso Sep 25, 2023

GustavoCaso Sep 25, 2023

ekump Sep 25, 2023

GustavoCaso commented Sep 25, 2023

ekump commented Sep 25, 2023

		@@ -32,12 +32,6 @@ Lint/MissingSuper:
		- 'lib/datadog/profiling/pprof/converter.rb'

		# Below should be:
		# require_relative '../../transport/http/api'

		# Below should be:
		# require_relative '../../../../ddtrace/transport/http/api/spec'

ekump/decouple core transport #3150

ekump/decouple core transport #3150

Conversation

ekump commented Sep 21, 2023 • edited Loading

marcotc Sep 22, 2023

Choose a reason for hiding this comment

marcotc left a comment

Choose a reason for hiding this comment

anmarchenko left a comment

Choose a reason for hiding this comment

GustavoCaso Sep 25, 2023

Choose a reason for hiding this comment

GustavoCaso Sep 25, 2023

Choose a reason for hiding this comment

GustavoCaso Sep 25, 2023

Choose a reason for hiding this comment

ekump Sep 25, 2023

Choose a reason for hiding this comment

GustavoCaso commented Sep 25, 2023

ekump commented Sep 25, 2023

ekump commented Sep 21, 2023 •

edited

Loading