-
Notifications
You must be signed in to change notification settings - Fork 369
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
Trace Sidekiq server internal events (heartbeat, job fetch, and scheduled push) #1685
Conversation
65b0f56
to
fc2a28f
Compare
Sorry for all of the CI failure noise here folks, it took some time to get things all squared away across various Sidekiq (and Ruby) versions! |
Codecov Report
@@ Coverage Diff @@
## master #1685 +/- ##
==========================================
- Coverage 98.31% 98.20% -0.11%
==========================================
Files 927 933 +6
Lines 44662 44804 +142
==========================================
+ Hits 43909 44000 +91
- Misses 753 804 +51
Continue to review full report at Codecov.
|
Thank you for the PR, @agrobbin!
No need to apologize! This is what CI is here for :) Feel free to push as many times as you'd like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work, @agrobbin! I've left a few comments for you to take a look.
def patch_server_internals | ||
# Sidekiq < 5.2.4 (before mperham/sidekiq@ddb0c8b3) doesn't require this until | ||
# too late in the process (after `CLI#run` has been called) | ||
require 'sidekiq/launcher' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We normally avoid requiring external gem code in ddtrace
, as to not affect gem initialization order in the host application. Some gems are picky with initialization order.
Also, if I'm reading this correctly, 'sidekiq/launcher' is only needed when running Sidekiq jobs, but not when simply enqueuing them. This means that many applications that only enqueue jobs would actually never need to require 'sidekiq/launcher'
.
With that in mind, what do you think of removing the require
here and instead making
patch_server_heartbeat
patch_server_job_fetch
patch_server_scheduled_push
conditional on having sidekiq/launcher
loaded? It seems to me that this would work for users of Sidekiq >= 5.2.4, which is over 2 years old, but not force us to explicitly load it for all users of ddtrace
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm more than happy to only do this for Sidekiq >= 5.2.4, but just to be clear, this won't attempt to load sidekiq/launcher
for all users of dd-trace, only those who call use :sidekiq
in their configuration. Does that change your opinion at all @marcotc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only those who call use :sidekiq in their configuration
For web applications, for example, they will have use :sidekiq
because they want to capture MyJob.perform_async
calls. But these applications will never require sidekiq/launcher
normally. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but we were only ever requiring sidekiq/launcher
inside a Sidekiq.configure_server
block, so it shouldn't have had a significant impact. Either way though, I've removed it!
lib/ddtrace/contrib/sidekiq/server_internal_tracer/job_fetch.rb
Outdated
Show resolved
Hide resolved
lib/ddtrace/contrib/sidekiq/server_internal_tracer/scheduled_push.rb
Outdated
Show resolved
Hide resolved
Appraisals
Outdated
gem 'sidekiq', '~> 3.5.4' | ||
gem 'sidekiq', '~> 4.0.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our test suite runs for all major Sidekiq versions, from 3.5 to 6.2.
I noticed that you needed to bump up only Ruby 2.1, which uses Sidekiq 3.5.4.
What's the impact of leaving this 3.5.4 support for Sidekiq? Is it possible to keep support at 3.5.4, while dynamically skipping new features introduced in this PR if necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can definitely do that! Out of curiosity, is there a particular rule of thumb DataDog follows for backwards compatibility? That'd be great to know for any future contributions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always try to not break existing behaviour: if it was working in version X, should work in version X+1.
If we want to improve things, but these improvements only work with newer versions of the integrated library (Sidekiq in our case), it's reasonable to have separate code paths: one for old behaviour, another for new and improved behaviour. We normally only do this for brand new features, not for two versions of an existing feature (unless strictly technically required).
So, for example, adding a few additional spans or additional tags for only more recent versions of Sidekiq is ok. But having, for example, the span.resource
for the main Sidekiq span changing depending on Sidekiq version is not great, as people will upgrade Sidekiq and all of the sudden their instrumentation has changed as well, breaking dashboards and monitors.
include Sidekiq::Worker | ||
def perform; end | ||
end) | ||
module SidekiqServerExpectations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super cool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It took a number of iterations to finally find the right combination of things that make this work, so hopefully it can be useful going forward!
930447a
to
38a1310
Compare
@marcotc I just pushed up an update that limits these server internal traces to SIdekiq v5.2.4+. Let me know what you think of my approach. |
38a1310
to
6752b5d
Compare
Everything looks great, @agrobbin! One last thing: did you get a chance to try this in your development/staging environment, to see if the new traces look good to you as a Sidekiq user? |
…uled push) These 3 areas of the open source version of Sidekiq are not wrapped in high-level spans, resulting in a *lot* of one-off Redis spans that are not related to one another in any way.
6752b5d
to
b01ea9c
Compare
@marcotc I just deployed it to our staging environment, and things look good! I've attached some screenshots of the traces: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much, @agrobbin!
Awesome @marcotc, looking forward to seeing this released! |
These 3 areas of the open source version of Sidekiq are not wrapped in high-level spans, resulting in a lot of one-off Redis spans that are not related to one another in any way.
The most important change to call out here is the bump in the supported Sidekiq version. There were some significant underlying code changes between Sidekiq v3 and v4, and with Sidekiq 3.5.4 being over 5 years old (released 2016-01-13), that seemed to be a reasonable decision.
That said, I'm open to keeping v3 support, with this new internal tracing focused only on v4+.
Closes #1673.