feat: Add ActiveStorage instrumentation #1313

ymtdzzz · 2024-12-20T07:30:20Z

Description

Adding support for instrumenting ActiveStorage using ActiveSupport::Notification.

related: #1307

Test

Example script

$ cd instrumentation/active_storage/example
$ ruby trace_demonstration.rb

#<struct OpenTelemetry::SDK::Trace::SpanData
 name="service_upload.active_storage",
 kind=:internal,
 status=#<OpenTelemetry::Trace::Status:0x000000012b2df308 @code=1, @description="">,
 parent_span_id="\x00\x00\x00\x00\x00\x00\x00\x00",
 total_recorded_attributes=2,
 total_recorded_events=0,
 total_recorded_links=0,
 start_timestamp=1736671214398000000,
 end_timestamp=1736671214398395000,
 attributes={"active_storage.checksum"=>"x4UGDIZnlswqFwjJlxVMjg==", "active_storage.service"=>"Disk"},
 links=nil,
 events=nil,
 resource=
  #<OpenTelemetry::SDK::Resources::Resource:0x000000011f5bd810
   @attributes=
    {"service.name"=>"unknown_service",
     "process.pid"=>98542,
     "process.command"=>"trace_demonstration.rb",
     "process.runtime.name"=>"ruby",
     "process.runtime.version"=>"3.2.5",
     "process.runtime.description"=>"ruby 3.2.5 (2024-07-26 revision 31d0f1a2e7) [arm64-darwin23]",
     "telemetry.sdk.name"=>"opentelemetry",
     "telemetry.sdk.language"=>"ruby",
     "telemetry.sdk.version"=>"1.6.0"}>,
 instrumentation_scope=#<struct OpenTelemetry::SDK::InstrumentationScope name="OpenTelemetry::Instrumentation::ActiveStorage", version="0.0.0">,
 span_id="\xCA$\xA1\xD6\x01RV\x95",
 trace_id="\x13~\xDF\x02\x1F\x8C\xF3\x8B\x96\xA0\x97\xC3)}\x9A\xFA",
 trace_flags=#<OpenTelemetry::Trace::TraceFlags:0x000000012a539050 @flags=1>,
 tracestate=#<OpenTelemetry::Trace::Tracestate:0x000000012a536508 @hash={}>>

With other instrumentations in your Rails app

Without ActiveStorage instrumentation

# config/initializers/opentelemetry.rb
require "opentelemetry/sdk"

# install all compatible instrumentation with default configuration
OpenTelemetry::SDK.configure do |c|
  c.service_name = "otel-ruby-development"
  c.use "OpenTelemetry::Instrumentation::ActionPack"
  c.use "OpenTelemetry::Instrumentation::ActiveRecord"
  # c.use "OpenTelemetry::Instrumentation::ActiveStorage"
  c.use "OpenTelemetry::Instrumentation::ActiveJob"
end

With ActiveStorage instrumentation

# config/initializers/opentelemetry.rb
require "opentelemetry/sdk"

# install all compatible instrumentation with default configuration
OpenTelemetry::SDK.configure do |c|
  c.service_name = "otel-ruby-development"
  c.use "OpenTelemetry::Instrumentation::ActionPack"
  c.use "OpenTelemetry::Instrumentation::ActiveRecord"
  c.use "OpenTelemetry::Instrumentation::ActiveStorage", { key: :include, url: :include }
  c.use "OpenTelemetry::Instrumentation::ActiveJob"
end

xuan-cao-swi · 2025-01-16T16:34:18Z

instrumentation/all/opentelemetry-instrumentation-all.gemspec

@@ -27,6 +27,7 @@ Gem::Specification.new do |spec|
  spec.required_ruby_version = '>= 3.0'

  spec.add_dependency 'opentelemetry-instrumentation-active_model_serializers', '~> 0.21.0'
+  spec.add_dependency 'opentelemetry-instrumentation-active_storage', '~> 0.0.0'


I think it's best to include this instrumentation to -all and -rails once this pr gets merged and released (other part of code that is related to this applies)

I think that's a good call. However, if this is added to -rails, I don't think it also has to be added to -all, because -all requires -rails.

Make sense. In d6979a7 , I removed -active_storage from -all .
I'll create another PR to -active_storage this to -rails after this PR is relesed.

xuan-cao-swi · 2025-01-16T16:36:42Z

instrumentation/active_storage/test/test_helpers/app_config.rb

+    when /^7\./
+      ActiveStorage::Current.url_options = { host: 'http://example.com' }
+    when /^8\./
+      ActiveStorage::Current.url_options = { host: 'http://example.com' }


nit: why they need to be on separate line?

Thank you for your advice, I fixed unnecessary conditions in bfae8c7 !

xuan-cao-swi · 2025-01-16T16:37:38Z

instrumentation/active_storage/lib/opentelemetry/instrumentation/active_storage/version.rb

+module OpenTelemetry
+  module Instrumentation
+    module ActiveStorage
+      VERSION = '0.0.0'


let's make it '0.1.0' as initial release version.

changed the version in d6979a7

kaylareopelle

Thank you, @ymtdzzz! I'm looking forward to OTel to offering ActiveStorage support.

kaylareopelle · 2025-01-17T23:41:49Z

instrumentation/active_storage/Appraisals

+#
+# SPDX-License-Identifier: Apache-2.0
+
+%w[6.1.0 7.0.0 7.1.0].each do |version|


Suggested change

%w[6.1.0 7.0.0 7.1.0].each do |version|

%w[7.0.0 7.1.0].each do |version|

We recently removed support for Rails 6.1. See: #1231

Thank you for pointing that out! I fixed in c6ea05f

kaylareopelle · 2025-01-18T00:19:11Z

...mentation/active_storage/lib/opentelemetry/instrumentation/active_storage/instrumentation.rb

+module OpenTelemetry
+  module Instrumentation
+    module ActiveStorage
+      # The {OpenTelemetry::Instrumentation::ActiveStorage::Instrumentation} class contains logic to detect and install the ActiveStorage instrumentation


Excellent docs! Thank you!!

kaylareopelle · 2025-01-18T00:27:29Z

...mentation/active_storage/lib/opentelemetry/instrumentation/active_storage/instrumentation.rb

+      #     })
+      #   end
+      class Instrumentation < OpenTelemetry::Instrumentation::Base
+        MINIMUM_VERSION = Gem::Version.new('6.1.0')


Like I mentioned above, our new minimum Rails version is 7.0 :)

Suggested change

MINIMUM_VERSION = Gem::Version.new('6.1.0')

MINIMUM_VERSION = Gem::Version.new('7.0.0')

This is also fixed in c6ea05f

kaylareopelle · 2025-01-18T01:07:28Z

...mentation/active_storage/lib/opentelemetry/instrumentation/active_storage/instrumentation.rb

+      # ## Configuration keys and options
+      #
+      # ### `:disallowed_notification_payload_keys`
+      #


It looks like the default is missing on just this config:

Suggested change

#

#

# - `array` **default** `[]`

#

I agree, fixed in 613e1b6

kaylareopelle · 2025-01-18T01:11:43Z

instrumentation/active_storage/opentelemetry-instrumentation-active_storage.gemspec

+  spec.add_development_dependency 'opentelemetry-sdk', '~> 1.1'
+  spec.add_development_dependency 'opentelemetry-test-helpers', '~> 0.3'
+  spec.add_development_dependency 'rake', '~> 13.0'
+  spec.add_development_dependency 'rubocop', '~> 1.69.1'


We're about to merge a PR that'll bump Rubocop to 1.70.0:

Suggested change

spec.add_development_dependency 'rubocop', '~> 1.69.1'

spec.add_development_dependency 'rubocop', '~> 1.70.0'

Thank you for letting me know, I've matched the version with other packages!
b98fadf

kaylareopelle · 2025-01-18T01:26:18Z

instrumentation/active_storage/test/test_helper.rb

+# SPDX-License-Identifier: Apache-2.0
+
+ENV['RAILS_ENV'] = 'test'
+


We recently had to add explicit requires to 'logger' in the test helpers for some Rails-related libraries to avoid uninitialized constant errors: #1345

Rails added logger as a dependency in Rails 7.1 and above, but since we still support 7.0, we need to require it here.

Suggested change

require 'logger'

That makes sense, I added explicit logger require. da97949

kaylareopelle · 2025-01-18T01:31:15Z

instrumentation/active_storage/test/test_helpers/app_config.rb

+    when /^6\.1/
+      ActiveStorage::Current.host = 'http://example.com'


Cleanup related to dropping 6.1 support:

Suggested change

when /^6\.1/

ActiveStorage::Current.host = 'http://example.com'

thanks, this is also fixed in c6ea05f !

kaylareopelle · 2025-01-18T01:32:19Z

instrumentation/all/opentelemetry-instrumentation-all.gemspec

@@ -27,6 +27,7 @@ Gem::Specification.new do |spec|
  spec.required_ruby_version = '>= 3.0'

  spec.add_dependency 'opentelemetry-instrumentation-active_model_serializers', '~> 0.21.0'
+  spec.add_dependency 'opentelemetry-instrumentation-active_storage', '~> 0.0.0'


I think that's a good call. However, if this is added to -rails, I don't think it also has to be added to -all, because -all requires -rails.

kaylareopelle · 2025-01-18T01:39:34Z

.github/actions/test_gem/action.yml

+    - name: Install ImageMagick for active_storage testing
+      if: "${{ inputs.gem == 'opentelemetry-instrumentation-active_storage' }}"
+      shell: bash
+      run: sudo apt update && sudo apt install -y imagemagick


Thanks for adding this service here! We also have a docker-compose.yml to allow developers to run services locally. Would you mind adding ImageMagick to that file too?

As you said, the test for ActiveStorage instrumentation did not pass due to the lack of ImageMagick inside the container. So I added it to the Dockerfile and confirmed that the test passed. ed63c88

❯ docker compose run app /bin/ash /app $ cd instrumentation/active_storage/ /app/instrumentation/active_storage $ bundle install /app/instrumentation/active_storage $ bundle exec appraisal install /app/instrumentation/active_storage $ bundle exec appraisal rake test ... # Running: .......................... Finished in 0.976136s, 26.6356 runs/s, 101.4203 assertions/s. 26 runs, 99 assertions, 0 failures, 0 errors, 0 skips

…th other packages

…ve active_storage from all

ymtdzzz force-pushed the feature/active_storage_support branch 6 times, most recently from 7a91acc to 641f823 Compare January 13, 2025 01:58

ymtdzzz changed the title ~~[WIP] feat: Add ActiveStorage instrumentation~~ feat: Add ActiveStorage instrumentation Jan 14, 2025

ymtdzzz force-pushed the feature/active_storage_support branch 2 times, most recently from 721c056 to 5959af8 Compare January 14, 2025 12:43

ymtdzzz added 3 commits January 14, 2025 23:22

feat: Add ActiveStorage instrumentation

e972502

fix: markdown lint error in README.md

04abd66

fix: rubocop error in opentelemetry-instrumentation-all.gemspec

cf7ee82

ymtdzzz force-pushed the feature/active_storage_support branch from 5959af8 to cf7ee82 Compare January 14, 2025 14:23

ymtdzzz marked this pull request as ready for review January 14, 2025 14:45

ymtdzzz requested review from fbogsany, mwear, robertlaurin, dazuma, ericmustin, arielvalentin, ahayworth, plantfansam, robbkidd, simi, kaylareopelle, xuan-cao-swi and a team as code owners January 14, 2025 14:45

xuan-cao-swi reviewed Jan 16, 2025

View reviewed changes

kaylareopelle requested changes Jan 18, 2025

View reviewed changes

ymtdzzz added 8 commits January 18, 2025 14:33

Merge branch 'main' into feature/active_storage_support

7fa010c

feat: remove support for Rails 6.1

c6ea05f

docs: add default value on :disallowed_notification_payload_keys

613e1b6

feat: match rubocop and opentelemetry-instrumentation-base version wi…

b98fadf

…th other packages

test: add explicit require to logger

da97949

test: remove unnecessary conditions

bfae8c7

chore: add imagemagick package to Dockerfile

ed63c88

feat: change active_storage instrumentation version to 0.1.0 and remo…

d6979a7

…ve active_storage from all

ymtdzzz requested review from kaylareopelle and xuan-cao-swi January 18, 2025 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ActiveStorage instrumentation #1313

feat: Add ActiveStorage instrumentation #1313

ymtdzzz commented Dec 20, 2024 •

edited

Loading

xuan-cao-swi Jan 16, 2025 •

edited

Loading

kaylareopelle Jan 18, 2025

ymtdzzz Jan 18, 2025

xuan-cao-swi Jan 16, 2025 •

edited

Loading

ymtdzzz Jan 18, 2025 •

edited

Loading

xuan-cao-swi Jan 16, 2025

ymtdzzz Jan 18, 2025

kaylareopelle left a comment

kaylareopelle Jan 17, 2025

ymtdzzz Jan 18, 2025

kaylareopelle Jan 18, 2025

kaylareopelle Jan 18, 2025

ymtdzzz Jan 18, 2025

kaylareopelle Jan 18, 2025

ymtdzzz Jan 18, 2025

kaylareopelle Jan 18, 2025

ymtdzzz Jan 18, 2025

kaylareopelle Jan 18, 2025

ymtdzzz Jan 18, 2025

kaylareopelle Jan 18, 2025

ymtdzzz Jan 18, 2025

kaylareopelle Jan 18, 2025

kaylareopelle Jan 18, 2025

ymtdzzz Jan 18, 2025

	%w[6.1.0 7.0.0 7.1.0].each do \|version\|
	%w[7.0.0 7.1.0].each do \|version\|

	MINIMUM_VERSION = Gem::Version.new('6.1.0')
	MINIMUM_VERSION = Gem::Version.new('7.0.0')

-      #
+      #
+      # - `array` **default** `[]`
+      #

	spec.add_development_dependency 'rubocop', '~> 1.69.1'
	spec.add_development_dependency 'rubocop', '~> 1.70.0'

		# SPDX-License-Identifier: Apache-2.0

		ENV['RAILS_ENV'] = 'test'

		when /^6\.1/
		ActiveStorage::Current.host = 'http://example.com'

feat: Add ActiveStorage instrumentation #1313

Are you sure you want to change the base?

feat: Add ActiveStorage instrumentation #1313

Conversation

ymtdzzz commented Dec 20, 2024 • edited Loading

Description

Test

Example script

With other instrumentations in your Rails app

Without ActiveStorage instrumentation

With ActiveStorage instrumentation

xuan-cao-swi Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuan-cao-swi Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

ymtdzzz Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

ymtdzzz commented Dec 20, 2024 •

edited

Loading

xuan-cao-swi Jan 16, 2025 •

edited

Loading

xuan-cao-swi Jan 16, 2025 •

edited

Loading

ymtdzzz Jan 18, 2025 •

edited

Loading