Add active storage instrumentation #1290
Conversation
| end) | ||
|
|
||
| stub_const('Article', Class.new(ApplicationRecord) do | ||
| # active storage initializers have changed from 5 => 6 so we need to mimic both approaches |
There was a problem hiding this comment.
We have different application files for different versions of Rails: https://github.com/DataDog/dd-trace-rb/tree/master/test/contrib/rails/apps
Do you think it makes sense to move this to rails5.rb and rails6.rb respectively?
It could be something like declaring a global method, like:
stub_const('Article', new_application_record)or a mixin:
stub_const('Article', Class.new(ApplicationRecord) do
include Datadog::Testing::ApplicationRecord
end)What do you think?
There was a problem hiding this comment.
makes sense, can clean this up
| # Rails.configuration.active_storage.service_configurations = { | ||
| # local: { | ||
| # service: 'Disk', | ||
| # root: '/tmp/dd-trace-rb/storage' | ||
| # } | ||
| # } |
There was a problem hiding this comment.
Is this block supposed to be commented out?
There was a problem hiding this comment.
err, i think it should've been deleted, will update
| # https://github.com/rails/rails/blob/2a3f8a29e5d7d17a0448fa47fd756c8d11b175b1/activestorage/app/models/active_storage/blob.rb#L64 | ||
| # https://github.com/rails/rails/blob/06c94818d640d13efecc722c4ec8c9c5ea2a99b0/activestorage/app/models/active_storage/blob.rb#L104 | ||
| def create_blob(data: 'Hello world!', filename: 'hello.txt', content_type: 'text/plain', identify: true, record: nil) | ||
| if Rails.version >= '6.0' |
There was a problem hiding this comment.
Similar question/suggestion about possibly moving this to https://github.com/DataDog/dd-trace-rb/tree/master/test/contrib/rails/apps.
There was a problem hiding this comment.
yea i think that's reasonable
| module Events | ||
| # Defines instrumentation for 'service_delete.active_storage' event. | ||
| # From: https://edgeguides.rubyonrails.org/active_support_instrumentation.html#active-storage | ||
| module Delete |
There was a problem hiding this comment.
The events are not all the same, but I see quite a few similarities between them.
Would it be possible to create a shared component for these?
I understand that not all the fields are the same, and would require custom treatment.
There was a problem hiding this comment.
yes probably, i can take an approach similar to our kafka instrumentation
delner
added
feature
Summary
This PR adds instrumentation for the Rails ActiveStorage gem, using the ActiveSupport Notifications that it emits.
ActiveStorage is available in both rails 5 and 6 for ruby versions >=2.5. By adding ActiveStorage instrumentation it allows the
railsintegration to have better coverage over possible rails deployments.Notes
The tests here are quite cumbersome as I wanted to actually mock the full ActiveStorage action lifecycle instead of just mocking an activesupport notification. It required an additional
appraisalblock and a lot of boilerplate. There are some lifecycle hooks that handle appending theActiveStoragemethods/macros to ActiveRecord as well as establishing/creating migrations for the underlying db tables that power ActiveStorage's file management. These lifecycle hooks changed pretty dramatically, including changes to file structure, between rails 5 and 6, so I had to mock all of this, and it can be a little distracting in the tests.I wasn't sure about the value of some of the instrumented events, but i added all of them as they each seemed to represent unique actions related to file management.