Add integration test suite #1305

delner · 2021-01-08T22:09:03Z

Goal for this pull request is to introduce an integration test suite that can run real applications (via Docker images) to test the ddtrace library. These tests will focus on stability ("will the application load & function?") and correctness ("do they produce the expected APM data?")

These tests should be available to run locally, and will automatically run via CircleCI upon a pull request or merge.

TODO:

Create Rack, Rails 5 test apps (integration/apps/rack, integrations/apps/rails-five)
Build Docker images for test apps and upload them to DockerHub (via DockerHub autobuilds)
Build Docker images for test apps in CI pipeline (to be used by the current pipeline)
Add scripts to run integration apps & tests in CI
Fill out test matrix (dimensions: Ruby version/integration app)
Write integration tests for each test application
~~Add additional test apps, e.g. Rails 6, Sinatra, Ruby (stretch goal)~~

ivoanjo

Thanks a lot for this, it will be mega useful :)

I wonder if it's worth leaving a TODO/FIXME to add a check for Ruby 2.0. Just so it's less hard to get surprised by something weird/broken there.

integration/README.md

integration/apps/rack/Gemfile

integration/apps/rails-five/.generators

integration/apps/rails-five/.rakeTasks

integration/apps/rails-five/config/initializers/inflections.rb

integration/apps/rails-six/config/initializers/application_controller_renderer.rb

integration/apps/sinatra/puma.rb

integration/images/2.2/Dockerfile

integration/images/include/datadog/demo_env.rb

integration/apps/sinatra/app.rb

codecov-io · 2021-01-14T03:01:09Z

Codecov Report

Merging #1305 (3a12fa5) into master (4c0d5e9) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1305   +/-   ##
=======================================
  Coverage   98.13%   98.13%           
=======================================
  Files         752      752           
  Lines       35745    35745           
=======================================
+ Hits        35078    35079    +1     
+ Misses        667      666    -1

Impacted Files	Coverage Δ
...b/action_pack/action_controller/instrumentation.rb	`95.77% <0.00%> (+1.40%)`	⬆️

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 4c0d5e9...3a12fa5. Read the comment docs.

delner · 2021-01-26T16:21:20Z

Removed rails-six, sinatra, ruby apps; will add these in another PR, but wanted to slim down this initial PR.

delner · 2021-01-26T17:18:41Z

All feedback should be addressed. I also cleaned up the history, trimmed out a bunch of files we're not using in this PR, and got the build mostly working.

Still one outstanding problem with Ruby 2.3: has a missing binary that should be there after gems are installed. I think for some reason the gems aren't being installed to /usr/local/bundle and are instead installed into the gems/2.3.0 directory, which might be resulting in a path issue (doesn't discover the installed binaries.) Haven't figured this one out yet, seems to only happen with 2.3.

Otherwise, this is ready for another round of code reviews. Should be a lot cleaner and easier to read this time around.

ivoanjo

Looking really nice! Left a bunch of small notes, but overall it's almost good to go :)

.circleci/config.yml

integration/README.md

integration/apps/rack/.byebug_history

integration/apps/rack/README.md

ivoanjo · 2021-01-27T12:17:49Z

integration/apps/rails-five/config/application.rb

+    # NOTE: Disabled for now, suspected to cause memory growth.
+    # Fetch from cache
+    # Rails.cache.fetch(request_id) { request_id }


Interesting... Can you add a bit more context here?

I think because each request ID is unique, and fetch gets or adds the request ID to the cache, it will grow indefinitely. This was added as a cheap way of implementing cache usage (for showing in traces) but it probably doesn't need to create this many keys.

Your explanation makes some sense :)
May I suggest adding it as a comment? That would be a good starting point if someone ran into this and was also wondering what was the problematic effect.

integration/apps/rails-five/config/puma.rb

ivoanjo · 2021-01-27T12:19:50Z

integration/apps/rails-five/config/puma.rb

+
+before_fork do
+  ActiveRecord::Base.connection_pool.disconnect!
+  #$redis.pool_shutdown { |conn| conn.quit }


I'm curious about this one, could you add a bit more detail?

I don't remember on this one... not sure if it was disabled for testing or if it was causing problems with restarting traces (e.g. conn.quit might be instrumented, which generates a span, which has to be emitted.)

integration/apps/rails-five/config/routes.rb

integration/images/ruby/3.0/Dockerfile

ivoanjo · 2021-02-05T10:22:18Z

integration/script/build-images

+  docker build -t datadog/dd-apm-demo:rb-2.0 -f $INTEGRATION_DIR/images/ruby/2.0/Dockerfile $INTEGRATION_DIR/images
+  docker build -t datadog/dd-apm-demo:rb-2.1 -f $INTEGRATION_DIR/images/ruby/2.1/Dockerfile $INTEGRATION_DIR/images
+  docker build -t datadog/dd-apm-demo:rb-2.2 -f $INTEGRATION_DIR/images/ruby/2.2/Dockerfile $INTEGRATION_DIR/images
+  docker build -t datadog/dd-apm-demo:rb-2.3 -f $INTEGRATION_DIR/images/ruby/2.3/Dockerfile $INTEGRATION_DIR/images
+  docker build -t datadog/dd-apm-demo:rb-2.4 -f $INTEGRATION_DIR/images/ruby/2.4/Dockerfile $INTEGRATION_DIR/images
+  docker build -t datadog/dd-apm-demo:rb-2.5 -f $INTEGRATION_DIR/images/ruby/2.5/Dockerfile $INTEGRATION_DIR/images
+  docker build -t datadog/dd-apm-demo:rb-2.6 -f $INTEGRATION_DIR/images/ruby/2.6/Dockerfile $INTEGRATION_DIR/images
+  docker build -t datadog/dd-apm-demo:rb-2.7 -f $INTEGRATION_DIR/images/ruby/2.7/Dockerfile $INTEGRATION_DIR/images
+  docker build -t datadog/dd-apm-demo:rb-3.0 -f $INTEGRATION_DIR/images/ruby/3.0/Dockerfile $INTEGRATION_DIR/images


Minor: It occurs to me that we could start leaving a comment like # SUPPORTED_RUBIES_UPDATE_HERE on every place in the codebase that would need to be touched e.g. once we drop support for 2.0 or once 3.1 gets released.

That way we could easily grep for that string whenever we're doing changes, and thus be reasonably sure we touched all the right places, rather than trying to grep 3.0 and 2.0 and hope we haven't missed anything :)

What do you think?

Yeah I do like a directive like that; would aid refactoring. Maybe that's a mini-RFC we should do for this application in general. Let's discuss with the rest of the Ruby team.

marcotc

🎉 I think this new test suite will greatly improve our confidence when introducing changes to the tracer. 🙇

delner added the dev/ci Involves CircleCI, GitHub Actions, or GitLab label Jan 8, 2021

delner requested review from marcotc, ivoanjo and ericmustin January 8, 2021 22:09

delner self-assigned this Jan 8, 2021

ivoanjo reviewed Jan 11, 2021

View reviewed changes

marcotc reviewed Jan 11, 2021

View reviewed changes

integration/apps/sinatra/app.rb Outdated Show resolved Hide resolved

delner force-pushed the ci/integration_suite branch 18 times, most recently from 4634b1b to b58f4f2 Compare January 13, 2021 23:11

delner force-pushed the ci/integration_suite branch 4 times, most recently from b8b14ed to a56049a Compare January 19, 2021 22:37

delner force-pushed the ci/integration_suite branch 4 times, most recently from 555167e to 07d1c5a Compare January 25, 2021 19:53

delner force-pushed the ci/integration_suite branch 2 times, most recently from 440b458 to 809b5d5 Compare January 26, 2021 17:10

delner marked this pull request as ready for review January 26, 2021 17:21

delner requested a review from a team January 26, 2021 17:21

ivoanjo reviewed Jan 27, 2021

View reviewed changes

delner force-pushed the ci/integration_suite branch 2 times, most recently from b67b240 to fccdcf8 Compare February 1, 2021 18:52

delner added 3 commits February 3, 2021 12:23

Added: Integration agent image.

7104aa7

Added: Include helpers for integration app images.

34f9216

Added: Wrk load tester image.

1d8d8e0

delner force-pushed the ci/integration_suite branch 2 times, most recently from db5ebd0 to 9d46a61 Compare February 4, 2021 19:37

delner added 6 commits February 4, 2021 14:42

Added: Integration app Ruby base images.

28e3d9e

Added: integration/script/build-images

ebfd43f

Added: Rack integration app.

2c1f845

Added: Rails 5 integration app.

5699c86

Added: Integration suite README

5da98a7

Added: CircleCI build steps for integration suite.

b934675

delner force-pushed the ci/integration_suite branch from 9d46a61 to b934675 Compare February 4, 2021 19:44

ivoanjo reviewed Feb 5, 2021

View reviewed changes

marcotc approved these changes Feb 17, 2021

View reviewed changes

delner merged commit e628a14 into master Feb 17, 2021

delner deleted the ci/integration_suite branch February 17, 2021 18:01

github-actions bot added this to the 0.46.0 milestone Feb 17, 2021

Add integration test suite #1305

Add integration test suite #1305

Uh oh!

Conversation

delner commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

delner commented Jan 26, 2021

Uh oh!

delner commented Jan 26, 2021

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivoanjo Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

delner Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

ivoanjo Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ivoanjo Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

delner Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ivoanjo Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

delner Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

marcotc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

delner commented Jan 8, 2021 •

edited

Loading

codecov-io commented Jan 14, 2021 •

edited

Loading