Feature / Introduce Que instrumentation #1146

hs-bguven · 2020-08-18T03:31:16Z

This PR introduces instrumentation for QUE library: https://github.com/que-rb/que

hs-bguven · 2020-08-18T04:00:32Z

First version of the PR. I would love to get your feedback on it. Looks like master has some failing tests atm.

I got it working on our staging env and see spans and traces pop up accordingly.

Now the question is, what do we do about this que gem since its 1.0.0.beta4? I can keep using a fork and hope for que folks push for a stable release... Any suggestions?

marcotc

Hey @hs-bguven, awesome work on this PR so far!
There are no fundamental issues, everything seems in good shape.

I left a few comments on some parts that I think will need some tweaking before we merge it.

I answered your question about using the beta version inline, in one of my comments, we can follow up the discussion there.

One thing I noticed is that there are no tests for the fields being captured as tags, nor tests for when the job raises an error, but I'm assuming that's why the PR is in draft mode :) We can flush out the testing coverage when that time comes, no rush.

Appraisals

marcotc · 2020-08-18T18:10:06Z

docs/GettingStarted.md

@@ -352,6 +353,7 @@ For a list of available integrations, and their configuration options, please re
 | MySQL2                   | `mysql2`                   | `>= 0.3.21`              | *gem not available*       | *[Link](#mysql2)*                   | *[Link](https://github.com/brianmario/mysql2)*                                 |
 | Net/HTTP                 | `http`                     | *(Any supported Ruby)*   | *(Any supported Ruby)*    | *[Link](#nethttp)*                  | *[Link](https://ruby-doc.org/stdlib-2.4.0/libdoc/net/http/rdoc/Net/HTTP.html)* |
 | Presto                   | `presto`                   | `>= 0.5.14`              | `>= 0.5.14`               | *[Link](#presto)*                   | *[Link](https://github.com/treasure-data/presto-client-ruby)*                  |
+| Que                      | `que`                      | `>= 2.2`                 | *gem not available*       | *[Link](#que)*                      | *[Link](https://github.com/que-rb/que)*                                        |


>= 2.2 should be updated to whatever version of que we'll end up supporting with this PR.
No need to update it now if you still don't know.

for some reason, I thought it was the min ruby version required by the gem. Updated it to match. Thank you.

No worries, thank you for addressing it.
For the JRuby column, where we currently have *gem not available*: if test-jruby-9.2 CI tests pass, then we can change that column to >= 1.0.0.beta2, as all functionality will have been tested with JRuby.

The gem itself claims it doesn't support Jruby. Would we still want to put >= 1.0.0.beta2 ??

Given your new tests pass, which exercise que and ddtrace together, I say we should state that we will be compatible with JRuby.

I found statement in the que repository: que-rb/que#4 (comment). It reads to me that que it self doesn't have any issues with JRuby support, but other dependencies might. The issue I liked is also very old (~7 years old) and we have been able to use JRuby with a Postgres driver successfully for a while.

Makes sense. Updated the jruby column to say >= 1.0.0.beta2.

Thank you for the clarification.

lib/ddtrace/contrib/que/integration.rb

marcotc · 2020-08-18T18:22:29Z

spec/ddtrace/contrib/que/tracer_spec.rb

+  class TestJobClass < ::Que::Job
+    def run(*args); end
+  end


Declaring a class here actually declares it on the global namespace, which is shared with all test cases.

On alternative we normally take in these cases is to either:

Not declare a class constant if not needed for the test:

let(:job_class) do Class.new(::Que::Job) do def run(*args); end end end

If a class constant is needed:

before do stub_const('TestJobClass', Class.new(::Que::Job) do def run(*args); end end) end

One reason a class constant might be needed is when a code path tries to read the class name: job_class.name will return nil, while TestJobClass.name will return "TestJobClass".

👍 makes sense.

I am going to have to carve out some this weekend to see if I can make a headway in the tests here. Que is giving me some hard time. Need to understand the inner workings of the gem a bit better to manipulate jobs in test setup. (the right mock setup.)

Objective:

Find a way to populate jobs so that I can assert something about each span I am collecting data for

cover failure cases for when things go wrong and etc..

Sounds good, no rush!

I noticed they provide some guidance on testing que: https://github.com/que-rb/que#testing
The suggested Que::Job.run_synchronously = true seem very promising. 🤞 it doesn't completely bypass middleware execution.

I have been doing something wrong initially but when I figured out my mistake Que::Job.run_synchronously = true worked like a charm.

lib/ddtrace/contrib/que/tracer.rb

…ove test

marcotc

👋 @hs-bguven, the PR looks pretty much ready to go, awesome work!

Is there anything that you still wanted to work on here?
I ask because the the PR, from the technical standpoint, looks ready! I don't see any missing parts, but as you are the production user of que here, I wanted to make sure you are happy with the changes yourself first.

marcotc · 2020-08-24T18:52:58Z

lib/ddtrace/contrib/que/tracer.rb

+          request_span = generate_spans(job)
+          set_sample_rate(request_span)
+          Contrib::Analytics.set_measured(request_span)
+
+          yield
+        rescue StandardError => e
+          request_span.set_error(e) unless request_span.nil?
+          raise e
+        ensure
+          request_span.finish if request_span


I noticed you are using the explicit pattern of: starting the trace, handling error, and finishing it explicitly.
This is a more brittle pattern than using the tracer.trace(...) { |span| yield } block, as the block ensures the spans is always finished and handles error propagation for you.

I don't see a strong reason to use the explicit operations, instead of the tracer.trace{} block syntax here, so I'd recommend the change the block pattern.

But, if you have a reason to keep this explicit pattern, please let me know and we can work with it no problem.

No particular reason, I went off the example I have seen in the sneaker tracer you suggested.

I myself find explicit style much more readable and maintainable since all the steps are available right in front of the developer to read and understand (less magic and tracing). But I would be more than happy to switch the pattern to comply with your repo style.

Thank you for pointing that out.

updated now : )

hs-bguven · 2020-08-26T06:00:29Z

👋 @hs-bguven, the PR looks pretty much ready to go, awesome work!

Is there anything that you still wanted to work on here?
I ask because the the PR, from the technical standpoint, looks ready! I don't see any missing parts, but as you are the production user of que here, I wanted to make sure you are happy with the changes yourself first.

Thank you for all the feedback @marcotc. I have been waiting for getting one final run of the changes in our staging to make sure everything worked as expected. As soon as I make the change for the tracer style I think the PR is good to go as well : ) 🤞

hs-bguven · 2020-08-26T06:24:26Z

all the changes have been made and tested in a live env with datadog. Looks all good.

If circleci passes we should be good to go : )

marcotc

Left one minor change, but I think that's all!

marcotc · 2020-08-26T17:05:48Z

Appraisals

@@ -369,6 +369,7 @@ elsif Gem::Version.new('2.2.0') <= Gem::Version.new(RUBY_VERSION) \
      gem 'sqlite3', '~> 1.3.6'
      gem 'sucker_punch'
      gem 'typhoeus'
+      gem 'que', '1.0.0.beta2'


Suggested change

gem 'que', '1.0.0.beta2'

gem 'que', '>= 1.0.0.beta2'

We like to keep these unlocked, as when the newest version gets released we are already testing against it.

That's my bad, totally overlooked that bit. Fixed now. Thank you for catching all these things.

marcotc · 2020-08-26T21:53:08Z

I'm rerunning the failed CI tests, as they seem unrelated.

marcotc

Thank you so much @hs-bguven! 🎉

By virtue of your production tests using this PR, it gives us a lot of confidence that new users will have no issues using it!

hs-bguven · 2020-08-27T18:34:29Z

@marcotc Thank you for all the support you have given me on this PR. This is going to solve a huge problem with our products.

marcotc reviewed Aug 18, 2020

View reviewed changes

hs-bguven added 4 commits August 20, 2020 22:19

Feature / Introduce Que instrumentation

94cff13

Update gem version requirement for que

4224772

update getting started doc

71f9f75

PR comment fixes: lower que version as low as possible, refactor, imr…

a6605e5

…ove test

hs-bguven force-pushed the feature/que-instrumentation branch from d3164b0 to a6605e5 Compare August 21, 2020 02:20

hs-bguven added 2 commits August 20, 2020 23:08

lint

19a302c

add test coverage for the tracer

81548ef

marcotc reviewed Aug 24, 2020

View reviewed changes

readme update, tracer style update

b76b19f

marcotc marked this pull request as ready for review August 26, 2020 17:04

marcotc requested a review from a team August 26, 2020 17:04

marcotc added community Was opened by a community member integrations Involves tracing integrations labels Aug 26, 2020

marcotc reviewed Aug 26, 2020

View reviewed changes

unlock the version number for the gem

a2fbe26

marcotc approved these changes Aug 27, 2020

View reviewed changes

marcotc added this to the 0.40.0 milestone Aug 27, 2020

marcotc merged commit aede812 into DataDog:master Aug 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature / Introduce Que instrumentation #1146

Feature / Introduce Que instrumentation #1146

hs-bguven commented Aug 18, 2020

hs-bguven commented Aug 18, 2020

marcotc left a comment

marcotc Aug 18, 2020

hs-bguven Aug 21, 2020

marcotc Aug 21, 2020

hs-bguven Aug 21, 2020

marcotc Aug 24, 2020

hs-bguven Aug 26, 2020

marcotc Aug 18, 2020

hs-bguven Aug 21, 2020

marcotc Aug 21, 2020

hs-bguven Aug 26, 2020

marcotc left a comment

marcotc Aug 24, 2020

hs-bguven Aug 26, 2020

hs-bguven Aug 26, 2020

hs-bguven commented Aug 26, 2020

hs-bguven commented Aug 26, 2020

marcotc left a comment

marcotc Aug 26, 2020

hs-bguven Aug 26, 2020

marcotc commented Aug 26, 2020

marcotc left a comment

hs-bguven commented Aug 27, 2020

Feature / Introduce Que instrumentation #1146

Feature / Introduce Que instrumentation #1146

Conversation

hs-bguven commented Aug 18, 2020

hs-bguven commented Aug 18, 2020

marcotc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hs-bguven commented Aug 26, 2020

hs-bguven commented Aug 26, 2020

marcotc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc commented Aug 26, 2020

marcotc left a comment

Choose a reason for hiding this comment

hs-bguven commented Aug 27, 2020