feat!(active_job): Use ActiveSupport instead of patches #677

arielvalentin · 2023-09-28T03:54:46Z

This PR changes the ActiveJob instrumentation to use ActiveSupport::Notifications instead of using Monkey Patches.

What's new?

The instrumentation now generates new internal spans for most of the ActiveJob notifications like discard and retry_stopped.
ActiveJob specific attributes are now namespaced under the messaging.active_job.*

Example Parent-Child propagation

Example Link Propagation

See #218

arielvalentin · 2023-09-30T02:34:21Z

Now that I have rewritten these to use AS Notifications I have been made aware of a few idiosyncratic things in rails 6 that makes it difficult to maintain parity with 7.

In Rails 6, perform_start is invoked prior to perform https://github.com/rails/rails/blob/v6.1.7.6/activejob/lib/active_job/instrumentation.rb#L13

The callbacks are called before the instrumentation events are invoked so they are not included in the timing
https://github.com/rails/rails/blob/v6.1.7.6/activejob/lib/active_job/execution.rb#L47

This is not ideal but in order to maintain correctness with 6.1 I will change this instrumentation to work with Rails 7+ and leave the original implementation in place for Rails 6.1.

Hopefully we have more luck with other framework components.

While working on open-telemetry#677, I ran into an issue where the base instrumentation tracer was not being automatically upgraded after the SDK initialization was complete. I am not sure why this was implemented this way but I figured other instrumentations would need a reference to a proxy tracer in the future. open-telemetry/opentelemetry-ruby#671

kaylareopelle

I'm still getting familiar with this code, so I will continue reviewing, but here's some low-level first-pass feedback.

kaylareopelle · 2023-10-09T17:23:28Z

instrumentation/active_job/Appraisals

-
-appraise 'activejob-7.0' do
-  gem 'activejob', '~> 7.0.0'
+%w[6.0.0 6.1.0 7.0.0 7.1.0].each do |version|


Did you leave 6.0 here on purpose, even though it's EOL'd?

Just keeping that's been merged thus far. The 6.0 was in a separate PR so I wanted to keep this the same until landing the other PR

instrumentation/active_job/README.md

instrumentation/active_job/lib/opentelemetry/instrumentation/active_job/handlers/default.rb

kaylareopelle · 2023-10-12T23:27:22Z

instrumentation/active_job/lib/opentelemetry/instrumentation/active_job.rb

@@ -11,6 +11,39 @@ module OpenTelemetry
  module Instrumentation
    # Contains the OpenTelemetry instrumentation for the ActiveJob gem
    module ActiveJob


I see a lot of duplication between this file and the Rack instrumentation file. It might be a good future candidate for a helper gem.

Yes!

I was of thinking of making a generic "ingress" span Context that would be instrumentation agnostic that way any child span is able to access the ingress span and enrich it.

So instead of saying Rack.current_span or ActiveJob.current_span it would be Ingress.current_span.

instrumentation/active_job/lib/opentelemetry/instrumentation/active_job/handlers/perform.rb

fbogsany · 2023-10-26T20:31:05Z

instrumentation/active_job/lib/opentelemetry/instrumentation/active_job/handlers.rb

+          tracer = OpenTelemetry.tracer_provider.tracer(ActiveJob.name, ActiveJob::VERSION)
+          mapper = Mappers::Attribute.new
+          config = ActiveJob::Instrumentation.instance.config
+          parent_span_provider = OpenTelemetry::Instrumentation::ActiveJob
+
+          # TODO, use delegation instead of inheritance
+          default_handler = Handlers::Default.new(tracer, parent_span_provider, mapper, config)
+          enqueue_handler = Handlers::Enqueue.new(tracer, parent_span_provider, mapper, config)
+          perform_handler = Handlers::Perform.new(tracer, parent_span_provider, mapper, config)


Our general pattern in instrumentation is to always reach for the tracer from the instrumentation when we need it rather than caching it in various helpers. I think the deviation from that code pattern here is the trigger for #688

Yessir! This was intentional on my part in the hopes of starting a new trend 😄

I wanted to use dependency injection for everything that the handlers needed instead of using a service locator or a mix of the two.

Though we are hashing out some details in #688, do you think having this asymmetry is problematic?

@fbogsany did you see my comment about this in the other issue?

Is this a deal breaker?

Given the results of the conversation in #688, I have changed the handlers to use the service locator pattern and look up the tracer from the Instrumentation singleton to creating spans.

bensheldon

I reviewed this PR, it looks correct. I'm excited to see more usage of Active Support Instrumentation. I'm assuming that once a few more components are instrumented, the common behavior will be extracted out into helpers and eventually slim this down.

btw, I'm the author of GoodJob; I also submitted the initial Active Job instrumentation for dd-trace-rb so I'm comparing and contrasting my experience implementing that to this code.

Of things I looked at:

I doublechecked the instrumentation names against my own lists, to make sure they were comprehensive.
I read through all the version-specific errata and couldn't think of any other weirdness (all the real weird weirdness I know of is in Rails <= 5.2, which isn't supported anyway)
I read through the tests and thought they poked at it in a good realistic / integrated fashion.

simi · 2023-10-30T19:34:57Z

@bensheldon is there anything GJ specific to instrument in separate instrumentation?

bensheldon · 2023-10-30T21:26:46Z

is there anything GJ specific to instrument in separate instrumentation?

I don't believe so, or anything that exists currently in GoodJob. The *.good_job events are all enumerated in this file.

Thinking about backend queue adapters generally there's usually an operation or two before perform.active_job that does the actual dequeuing operation. For example in GoodJob, within an execution thread there is a database query to the database to dequeue a job record, and then that record is passed into Active Job which then deserializes and performs it. Relatedly, I'm not sure if GlobalID deserialization happens as part of perform.active_job or before it. (GoodJob doesn't currently have instrumentation hooks at that foundational level... though it should!)

instrumentation/active_job/lib/opentelemetry/instrumentation/active_job/handlers/default.rb

instrumentation/active_job/README.md

arielvalentin · 2023-11-21T20:48:53Z

Up to date screenshots:

robbkidd

👍🏻

lavoiesl · 2024-07-22T13:40:49Z

instrumentation/active_job/lib/opentelemetry/instrumentation/active_job/handlers.rb

+        # Removes Event Handler Subscriptions for ActiveJob notifications
+        # @note this method is not thread-safe and should not be used in a multi-threaded context
+        def unsubscribe
+          @subscriptions&.each { |subscriber| ActiveSupport::Notifications.unsubscribe(subscriber) }


This needs to be ::ActiveSupport::Notifications, otherwise it throws:

uninitialized constant OpenTelemetry::Instrumentation::ActiveSupport::Notifications

Similarly, would you be able to provide us with a PR to address this problem?

lavoiesl · 2024-07-22T13:55:39Z

instrumentation/active_job/lib/opentelemetry/instrumentation/active_job/handlers/enqueue.rb

+            @span_name_formatter = if @config[:span_naming] == :job_class
+                                     ->(job) { "#{job.class.name} publish" }
+                                   else
+                                     ->(job) { "#{job.queue_name} publish" }
+                                   end


This is causing a regression in our codebase because the handler no longer honours dynamic configuration.

i.e. Changing OpenTelemetry::Instrumentation::ActiveJob::Instrumentation.instance after this is registered no longer affects the naming.

Arguably, we probably shouldn't rely on dynamic configs, but I wanted to point it out regardless

Same applies to Perform

Would you be able to provide us with a PR to fix this issue?

lavoiesl · 2024-08-20T21:30:29Z

instrumentation/active_job/lib/opentelemetry/instrumentation/active_job.rb

+      #
+      # @param [Span] span the span to activate
+      # @yield [span, context] yields span and a context containing the span to the block.
+      def with_span(span)


I think this is never called and that the context is never set.

This is causing link propagation to fail.

Do you have any additional details you would like to share in an issue?

Here we go! #1131

I was working on that, but it took me longer than expected to write up the issue. 😅

arielvalentin force-pushed the active-job-instrumentation-redux branch from 9599f18 to 4c659cc Compare September 28, 2023 04:05

arielvalentin force-pushed the active-job-instrumentation-redux branch 2 times, most recently from 3be71a3 to 4deba54 Compare October 11, 2023 01:45

arielvalentin changed the title ~~refactor(active_job): Use ActiveSupport instead of patches~~ feat!(active_job): Use ActiveSupport instead of patches Oct 11, 2023

arielvalentin force-pushed the active-job-instrumentation-redux branch from 4deba54 to 6f51c7c Compare October 11, 2023 02:01

arielvalentin self-assigned this Oct 11, 2023

arielvalentin marked this pull request as ready for review October 11, 2023 02:13

arielvalentin requested review from fbogsany, mwear, robertlaurin, dazuma, ericmustin, ahayworth, plantfansam and robbkidd as code owners October 11, 2023 02:13

arielvalentin mentioned this pull request Oct 11, 2023

refactor: Default to Proxy Tracer in instrumentations #688

Closed

arielvalentin requested review from simi and kaylareopelle October 13, 2023 22:00

kaylareopelle reviewed Oct 14, 2023

View reviewed changes

fbogsany reviewed Oct 26, 2023

View reviewed changes

bensheldon reviewed Oct 30, 2023

View reviewed changes

arielvalentin added 5 commits October 31, 2023 21:51

feat!(active_job): Use ActiveSupport instead of patches

55e11e3

refactor: Register discard job

44cb141

refactor: independent registration

9e20af7

refactor: Use template methods for children

76dd2a3

refactor: some more small fixes

f710452

arielvalentin added 4 commits November 21, 2023 10:33

squash: fix bad commit

c7f84b8

squash: Remove possible PII from status message

8237302

squash: PR Feedback

0287169

squash: Use up to date semconv and remove unused attrs

c0655eb

arielvalentin requested review from robbkidd and kaylareopelle November 21, 2023 16:53

squash: remove unused file

50ac4b5

arielvalentin requested a review from xuan-cao-swi November 21, 2023 16:55

squash: fix test

e3e096c

arielvalentin commented Nov 21, 2023

View reviewed changes

instrumentation/active_job/lib/opentelemetry/instrumentation/active_job/handlers/default.rb Outdated Show resolved Hide resolved

arielvalentin commented Nov 21, 2023

View reviewed changes

instrumentation/active_job/README.md Outdated Show resolved Hide resolved

arielvalentin and others added 4 commits November 21, 2023 12:43

squash: fix messaging id

071dc7a

squash: More semconv fixes

7639556

squash: fix ids

11799a9

squash: feedback from Ruby SIG

7235421

arielvalentin commented Nov 21, 2023

View reviewed changes

instrumentation/active_job/README.md Show resolved Hide resolved

robbkidd approved these changes Nov 21, 2023

View reviewed changes

kaylareopelle approved these changes Nov 21, 2023

View reviewed changes

arielvalentin merged commit fb31945 into open-telemetry:main Nov 22, 2023
47 checks passed

arielvalentin deleted the active-job-instrumentation-redux branch November 22, 2023 03:01

arielvalentin mentioned this pull request Nov 22, 2023

release: Release 6 gems #724

Merged

lavoiesl reviewed Jul 22, 2024

View reviewed changes

This was referenced Jul 22, 2024

Active job handlers no longer honour configuration changes #1077

Closed

fix(active-job): Honour dynamic changes in configuration #1079

Merged

feat(active-job)!: Normalize event messages #1080

Merged

lavoiesl reviewed Aug 20, 2024

View reviewed changes

This was referenced Aug 20, 2024

active_job: Context propagation is broken #1131

Closed

fix(active-job): Propagate context between enqueue and perform #1132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!(active_job): Use ActiveSupport instead of patches #677

feat!(active_job): Use ActiveSupport instead of patches #677

arielvalentin commented Sep 28, 2023 •

edited

Loading

arielvalentin commented Sep 30, 2023

kaylareopelle left a comment

kaylareopelle Oct 9, 2023

arielvalentin Oct 14, 2023

kaylareopelle Oct 12, 2023

arielvalentin Oct 14, 2023

fbogsany Oct 26, 2023

arielvalentin Oct 26, 2023 •

edited

Loading

arielvalentin Oct 31, 2023

arielvalentin Nov 1, 2023

bensheldon left a comment

simi commented Oct 30, 2023

bensheldon commented Oct 30, 2023

arielvalentin commented Nov 21, 2023

robbkidd left a comment

lavoiesl Jul 22, 2024

arielvalentin Jul 22, 2024

lavoiesl Jul 22, 2024

lavoiesl Jul 22, 2024

arielvalentin Jul 22, 2024

lavoiesl Jul 22, 2024

lavoiesl Aug 20, 2024

arielvalentin Aug 20, 2024

lavoiesl Aug 20, 2024

feat!(active_job): Use ActiveSupport instead of patches #677

feat!(active_job): Use ActiveSupport instead of patches #677

Conversation

arielvalentin commented Sep 28, 2023 • edited Loading

What's new?

Example Parent-Child propagation

Example Link Propagation

arielvalentin commented Sep 30, 2023

kaylareopelle 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

arielvalentin Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bensheldon left a comment

Choose a reason for hiding this comment

simi commented Oct 30, 2023

bensheldon commented Oct 30, 2023

arielvalentin commented Nov 21, 2023

robbkidd 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

arielvalentin commented Sep 28, 2023 •

edited

Loading

arielvalentin Oct 26, 2023 •

edited

Loading