add otel instrumentation #161
Conversation
lib/app_profiler.rb
Outdated
| def otel_instrumentation_enabled=(value) | ||
| if value | ||
| raise ArgumentError, | ||
| "OpenTelemetry::Instrumentation::Rack not defined" unless defined?(OpenTelemetry::Instrumentation::Rack) |
There was a problem hiding this comment.
Just a comment for posterity. The OpenTelemetry::Instrumentation::Rack module allows us to reliably get a handle on the rack span.
bmansoob
force-pushed
the
otel-instrumentation
branch
from
f861989 to
fb2cfef
Compare
|
@gmcgibbon if you could please take a look from ruby/rails perspective. Much appreciated 🙏 |
| @@ -50,5 +50,13 @@ class ConfigurationTest < TestCase | |||
| ensure | |||
| AppProfiler.profile_sampler_enabled = old_status | |||
| end | |||
|
|
|||
| test "otel_instrumentation_enabled raises ArgumentError if opentelemetry is not installed" do | |||
There was a problem hiding this comment.
will update the description since we are no longer raising.
There was a problem hiding this comment.
Hmmmm... With the approach I'm suggesting, we can likely mock the gem call to raise Gem::MissingSpecError instead. Though, this is usually tested with multiple bundles and test suites for when we have X gem and when we don't. Considering this is a small feature, I think mocking is ok.
There was a problem hiding this comment.
Mocking does not work -- because gem is monkey patched here? https://github.com/rubygems/rubygems/blob/bundler-v2.5.9/bundler/lib/bundler/rubygems_integration.rb#L206.
I can remove the test altogether and just rescue Gem::MissingSpecError and Gem::LoadError in code and leave at that.
lib/app_profiler.rb
Outdated
| logger.warn("OpenTelemetry::Instrumentation::Rack not defined. Setting enabled to false.") | ||
| @otel_instrumentation_enabled = false | ||
| return | ||
| end |
There was a problem hiding this comment.
This should raise when not available. We can implement a peer dependency with this:
gem "opentelemetry-instrumentation-rack", "VERSION_CONSTRAINT_HERE"
require "opentelemetry-instrumentation-rack"In place of the defined? call. Rails does something similar with Active Record adapters.
There was a problem hiding this comment.
I was trying to explicitly add a dependency to otel stuff, thinking that might be the preference. This is okay with me too.
There was a problem hiding this comment.
clarified privately -- I understand the suggestion now.
There was a problem hiding this comment.
Looking at this https://github.com/rubygems/rubygems/blob/master/lib/rubygems/core_ext/kernel_gem.rb#L11-L12. Since we do not need a version constraint, only require should suffice.
There was a problem hiding this comment.
| @@ -50,5 +50,13 @@ class ConfigurationTest < TestCase | |||
| ensure | |||
| AppProfiler.profile_sampler_enabled = old_status | |||
| end | |||
|
|
|||
| test "otel_instrumentation_enabled raises ArgumentError if opentelemetry is not installed" do | |||
There was a problem hiding this comment.
Hmmmm... With the approach I'm suggesting, we can likely mock the gem call to raise Gem::MissingSpecError instead. Though, this is usually tested with multiple bundles and test suites for when we have X gem and when we don't. Considering this is a small feature, I think mocking is ok.
| @@ -7,6 +7,11 @@ | |||
|
|
|||
| module AppProfiler | |||
| class Middleware | |||
| OTEL_PROFILE_ID = "profile.id" | |||
There was a problem hiding this comment.
We recently integrated profiling with tracing and the feedback we have gotten is quite encouraging.
The ability to see traces and profiles side by side is something internal users have asked for sometime now. To that end, It makes sense to integrate tracing and profiling here in the gem - to avoid repetitive rework in clients.
This PR adds a new
otel_instrumentation_enabledas a new config option. It can only be enabled ifOpenTelemetry::Instrumentation::Rackis loaded.app_profilerdoes not require this explicitly, it is optional.All our apps use
otelgems - so things should work out of the box for them.