Improve labeling of Sidekiq traces when using Delayed Extensions #798
Conversation
| @@ -446,6 +446,7 @@ elsif Gem::Version.new('2.3.0') <= Gem::Version.new(RUBY_VERSION) \ | |||
| gem 'dalli' | |||
| gem 'delayed_job' | |||
| gem 'delayed_job_active_record' | |||
| gem 'ruby-prof', '< 1.0' | |||
There was a problem hiding this comment.
I had to add this for the contrib appraisal to work. ruby-prof 1.0 requires ruby >=2.4.0.
joeyAghion
force-pushed
the
sidekiq-delayed-extensions
branch
from
da1eabc
to
29c9efb
Compare
|
Hey @joeyAghion, this looks like a nice feature! I'll try to give this a look over, and a review. In the meantime I would look into the failing tests, see if we can get those fixed up. |
delner
added
community
| @@ -18,6 +18,8 @@ def initialize(options = {}) | |||
| def job_resource(job) | |||
| if job['wrapped'] | |||
| job['wrapped'] | |||
| elsif job['class'] == 'Sidekiq::Extensions::DelayedClass' | |||
| YAML.load(job['args'].first)[0..1].join('.') rescue job['class'] # rubocop:disable Security/YAMLLoad | |||
There was a problem hiding this comment.
Looks like YAML.load is dangerous. Can you use safe_load instead?
There was a problem hiding this comment.
It's possible for YAML.load to fail if queued jobs are out of sync with loaded code, or for it to be unsafe if jobs were queued by untrusted sources, but I don't think it's possible to swap in safe_load here because we can't constrain the classes that these jobs may refer to. Queueing work for arbitrary classes is the power of Sidekiq's delayed extensions, and Sidekiq's own code uses load in that same unsafe way (if I understand it correctly).
I totally understand if you think this convenience isn't worth the potential risk or the rubocop exception. I wasn't sure of a better solution though.
There was a problem hiding this comment.
The built-in YAML parser, psych, doesn't like parsing that YAML payload in safe mode, like @joeyAghion mentioned:
[1] pry(main)> require 'yaml'
[2] pry(main)> YAML.safe_load(File.read('sidekiq_delayed_class.yaml'))
Psych::DisallowedClass: Tried to load unspecified class: ClientTracerTest::DelayableClass
from ~/.rbenv/versions/2.5.1/lib/ruby/2.5.0/psych/class_loader.rb:97:in `find'But safe_yaml works correctly:
[1] pry(main)> require 'safe_yaml'
[2] pry(main)> YAML.safe_load(File.read('sidekiq_delayed_class.yaml'))
=> ["ClientTracerTest::DelayableClass", ":do_work", []]For context, safe_yaml is a popular gem (with as many downloads as rubocop from rubygems, for reference), without any transient dependencies. But including a new gem as dependency to dd-trace-rb is always a concern.
There was a problem hiding this comment.
Hmmm, yeah, having to add a dependency like this is not great, especially if gem 'safe_yaml' might interfere with users' applications which use YAML: we wouldn't want tracing to suddenly swap out their YAML parser.
I'm open to other suggestions, but I don't think changing out YAML or using a function that exposes arbitrary code execution is a good idea. Is there another way to do this?
There was a problem hiding this comment.
Yeah this is a tough one. In our code base jobs are "trusted" but I could imagine other consumers not wanting to adopt that assumption.
I'll close this PR. Thanks for the feedback in any case!
If there's any interest, I could fashion our patch into a tiny extension gem that other users of delayed extensions could choose to include.
There was a problem hiding this comment.
Alright, bummer that there isn't any other way (was hoping maybe we could regex it or something), but feel free to make a small extension gem!
There was a problem hiding this comment.
I considered regex or other string-slicing but expect it would be uglier and depend on even more assumptions. Something like:
job['args'].first.match(/^---\n- !ruby\/class '(?<class>.*)'\n- :(?<method>.*)\n-/m) # => #<MatchData "..." class:"String" method:"to_s">I'd rather not add this imperfect YAML-parsing to the library, personally.
|
Closing as discussed. If others are interested in the patch I've posted it [and can theoretically keep it up to date] here: https://gist.github.com/joeyAghion/c109ffb6234df6417c1332741e85a0d0 |
We rely heavily on Sidekiq's delayed extensions to background any non-request work without needing to define lots of trivial worker classes.
As a result, practically all of the traces for our sidekiq service in Datadog are lumped under a single label:
We have been running with this small patch to improve the labeling and now get much more granular traces:
The patch is based on similar code in Sidekiq's job class. I'd have used that helper directly except that this middleware only has access to the job payload and not a job object.
Open to better (or just more efficient) ways of accomplishing this, as well as any other feedback.