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.

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

Refactor Datadog::HTTPTransport #628

Merged
merged 18 commits into from
Jun 27, 2019
Merged

Conversation

delner
Copy link
Contributor

@delner delner commented Nov 10, 2018

The Datadog::HTTPTransport class is quite large and contains a lot of responsibility, which makes it difficult to test, and to extend.

The goal for this pull request is to refactor the HTTPTransport class to accomplish a few things:

  • Make the transport simpler: Break it into smaller pieces so those pieces are easier to understand and use.
  • Make it easier to test: By virtue of having smaller pieces, it should be easier to test the behavior of discrete parts.
  • Make it easier to use in tests: Some tests want to stub requests to the agent completely, others want to mock and inspect the expected HTTP output. By substituting in a test component at the last step of the transport, we can accomplish both of these goals while still running a maximal amount of the typical code paths.
  • Make it easier to extend: As we develop improved means of sending data to the agent (e.g. Keep-Alives, connection pooling, alternative HTTP libraries, etc), having a transport layer made of simpler, interchangeable, extendable parts will make implementation and maintenance of such strategies easier.
  • Keep it performant: Maintain or improve performance vs the old transport.

What's left to do:

  • Add new Transport::HTTP components
  • Add a compatibility layer to new transport (so it can be interchanged with the old)
  • Add complimentary test components for new transport
  • Run performance comparison of old vs new transport
  • Update Writer to use new transport
  • Update old tests
  • Add metrics from Add internal tracing metrics #587 (Won't do; will revisit this later)
  • Add unit tests
  • Deprecate old transport

@delner delner added core Involves Datadog core libraries do-not-merge/WIP Not ready for merge dev/refactor Involves refactoring existing components labels Nov 10, 2018
@delner delner self-assigned this Nov 10, 2018
@delner
Copy link
Contributor Author

delner commented Nov 10, 2018

Ran some basic performance tests, which run the old transport and new transport end-to-end with the agent:

TL;DR New and old transport have comparable performance.

Old transport:

it 'HTTPTransport' do
  transport = Datadog::HTTPTransport.new(
    ENV.fetch('TEST_DDAGENT_HOST', 'localhost'),
    ENV.fetch('TEST_DDAGENT_PORT', 8126)
  )

  Benchmark.bm do |x|
    x.report('HTTPTransport') do
      1000.times do
        traces = get_test_traces(2)
        transport.send(:traces, traces)
      end
    end
  end
end
Randomized with seed 33851
       user     system      total        real
HTTPTransport  0.710000   0.240000   0.950000 (  1.051479)
.

Finished in 1.05 seconds (files took 0.55395 seconds to load)
1 example, 0 failures

New transport:

it 'Transport::HTTP' do
  client = Datadog::Transport::HTTP::Client.new(
    Datadog::Transport::HTTP::Service.new(
      ENV.fetch('TEST_DDAGENT_HOST', 'localhost'),
      ENV.fetch('TEST_DDAGENT_PORT', 8126)
    )
  )

  Benchmark.bm do |x|
    x.report('Transport::HTTP') do
      1000.times do
        traces = get_test_traces(2)
        parcel = Datadog::Transport::Traces::Parcel.new(traces)
        client.deliver(parcel)
      end
    end
  end
end
Randomized with seed 12812
       user     system      total        real
Transport::HTTP  0.620000   0.260000   0.880000 (  0.979254)
.

Finished in 0.98074 seconds (files took 0.55062 seconds to load)
1 example, 0 failures

@delner delner changed the title Refactor/transport components Refactor Datadog::HTTPTransport Nov 10, 2018
@delner delner force-pushed the refactor/transport_components branch from 3d479f3 to 6b20000 Compare November 11, 2018 17:44
@delner delner force-pushed the refactor/transport_components branch 2 times, most recently from a9f2059 to de5ca6b Compare December 18, 2018 21:18
@delner delner changed the base branch from 0.18-dev to 0.19-dev December 18, 2018 21:19
@delner delner force-pushed the refactor/transport_components branch from de5ca6b to 6aff632 Compare January 3, 2019 20:21
@delner delner force-pushed the refactor/transport_components branch from 6aff632 to d5fb2d7 Compare January 23, 2019 15:42
@delner delner changed the base branch from 0.19-dev to 0.20-dev January 23, 2019 16:30
@delner delner force-pushed the refactor/transport_components branch from d5fb2d7 to a983c13 Compare March 14, 2019 20:21
@delner delner changed the base branch from 0.20-dev to 0.21-dev March 14, 2019 20:22
@delner delner force-pushed the refactor/transport_components branch from a983c13 to 7ef020c Compare March 14, 2019 20:29
@delner delner force-pushed the refactor/transport_components branch from 7ef020c to 26eefa1 Compare March 20, 2019 22:13
@delner delner changed the base branch from 0.21-dev to 0.22-dev March 20, 2019 22:14
Copy link
Contributor

@pawelchcki pawelchcki left a comment

Choose a reason for hiding this comment

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

The new design looks quite clean! Great job @delner

@delner
Copy link
Contributor Author

delner commented Apr 23, 2019

#738 removed services from the trace library. This PR should be rebased on those changes, and the corresponding service components removed.

@delner delner force-pushed the refactor/transport_components branch from 26eefa1 to 5fdce05 Compare April 25, 2019 18:38
@delner delner changed the base branch from 0.22-dev to 0.23-dev April 25, 2019 18:39
@delner
Copy link
Contributor Author

delner commented Apr 25, 2019

Rebased this PR on 0.23-dev and removed services from it. Seems to be working okay.

lib/ddtrace/transport/http/client.rb Outdated Show resolved Hide resolved
lib/ddtrace/transport/http/client.rb Outdated Show resolved Hide resolved
lib/ddtrace/transport/http/service.rb Outdated Show resolved Hide resolved
@delner delner force-pushed the refactor/transport_components branch from 1bbf6f7 to 74c8576 Compare April 30, 2019 18:44
@delner delner changed the base branch from 0.23-dev to 0.24-dev April 30, 2019 18:45
@delner delner force-pushed the refactor/transport_components branch from d0a24a3 to 5b89986 Compare June 21, 2019 17:57
@delner delner force-pushed the refactor/transport_components branch from 5b89986 to e2f949b Compare June 21, 2019 18:42
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

lgtm

spec/ddtrace/transport/http/benchmark_spec.rb Outdated Show resolved Hide resolved
@delner delner added this to the 0.25.0 milestone Jun 24, 2019
Copy link
Contributor

@pawelchcki pawelchcki left a comment

Choose a reason for hiding this comment

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

One small question about using an uninstrumented copy of Net::HTTP for transport, otherwise looks good.
Love the new code coverage

lib/ddtrace/contrib/http/circuit_breaker.rb Show resolved Hide resolved
@delner delner force-pushed the refactor/transport_components branch from 0c6e4f1 to 84430ff Compare June 25, 2019 14:17
@delner delner merged commit f93a584 into 0.25-dev Jun 27, 2019
@delner delner deleted the refactor/transport_components branch August 12, 2019 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries dev/refactor Involves refactoring existing components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants