-
Notifications
You must be signed in to change notification settings - Fork 204
feat: add gem for auto-instrumentation in otel-operator #1384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: add gem for auto-instrumentation in otel-operator #1384
Conversation
We don't need to push to make the gem ready/release, but the script file need to get some review first. |
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
end | ||
|
||
# this is used for case when user particularly want to enable single instrumentation | ||
OTEL_INSTRUMENTATION_MAP = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update this list every time we add a new gem? If so, is this something we could add to the instrumentation generator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option is for convenience of only required certain libraries for instrumentation (idea is borrowed from nodejs). We don't necessarily need to have it if it's too much.
Other option is having something like
OTEL_INSTRUMENTATION_MAP = {}
OpenTelemetry::Instrumentation.constants.each do |const|
OTEL_INSTRUMENTATION_MAP[const.to_s] = "OpenTelemetry::Instrumentation::#{const}"
end
OTEL_INSTRUMENTATION_MAP['NET_HTTP'] = "OpenTelemetry::Instrumentation::Net::HTTP"
OTEL_INSTRUMENTATION_MAP.delete(:Base)
OTEL_INSTRUMENTATION_MAP.delete(:All)
output:
{"HTTP"=>"OpenTelemetry::Instrumentation::HTTP",
"DelayedJob"=>"OpenTelemetry::Instrumentation::DelayedJob",
"Ethon"=>"OpenTelemetry::Instrumentation::Ethon",
...
I am not sure if user prefer having the capitcal naming or all downcase (e.g. DelayedJob vs delayed_job)
e.g.
export OTEL_RUBY_ENABLED_INSTRUMENTATIONS=delayed_job,action_mailer
# or
export OTEL_RUBY_ENABLED_INSTRUMENTATIONS=DelayedJob,ActionMailer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do really like this and wished that the registry would support it. We only need a fully qualified name if the gem does not match the existing module pattern.
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
…lemetry-ruby-contrib into auto-instrumentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few observations and thoughts here for you. I do not want to block progress on this PR due to limited availability so I am not going to "request changes" but I would like you to strongly consider making changes to isolate the methods defined in the OTelBundlerPatch
.
Nit: The upstream docs uses mixed language but the headings at least say "Zero-Code instrumentation" as opposed to auto-instrumentation. How would you feel about naming this gem zero-code
as opposed to auto
?
I would also like to ask @simi their opinion here to confirm any bundler changes or see if there is another means of hooking this into Bundler.
@@ -214,6 +214,7 @@ def initialize(name, version, install_blk, present_blk, | |||
# | |||
# @param [Hash] config The config for this instrumentation | |||
def install(config = {}) | |||
puts "name: #{@name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you intend for this change to be present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
.github/workflows/ci-contrib.yml
Outdated
bundle exec rake test | ||
rm -f Gemfile.lock | ||
working-directory: './opentelemetry-auto-instrumentation' | ||
- name: "Test Ruby 3.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ruby 3.1 was EoL 2025-03-31? Want to still support it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
# Copyright The OpenTelemetry Authors | ||
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Consider using bundler inline like the other ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If use autoinstrumentation, then there is no need to do Bundler.require here since the autoinstrumentation script will do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think though that what we want is to see what "real world" applications would do. They require bundler and we are hooking into that flow. So event though we are requiring bundler as part of the Ruby run, wouldn't we want our examples to reflect how people would use it?
end | ||
OpenTelemetry.logger.info { 'Auto-instrumentation initialized' } | ||
rescue StandardError => e | ||
puts "Auto-instrumentation failed to initialize. Error: #{e.message}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you send this to $stderr
since it is a diagnostic message?
puts "Auto-instrumentation failed to initialize. Error: #{e.message}" | |
$stderr.puts "Auto-instrumentation failed to initialize. Error: #{e.message}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rubocop seems prefer warn; but I can disable that warning and use $stderr.puts.
end | ||
|
||
after do | ||
OTelBundlerPatch.send(:remove_const, :OTEL_INSTRUMENTATION_MAP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A side effect of localizing the methods. will mean only having to remove a single method from Bundler patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on this (I updated the test case based on nested module update)?
['LICENSE'] | ||
|
||
spec.require_paths = ['lib'] | ||
spec.required_ruby_version = '>= 3.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simiilar question mentioned above regarding support for Ruby 3.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
# /otel-auto-instrumentation-ruby is set in opentelemetry-operator ruby.go | ||
operator_gem_path = ENV['OTEL_RUBY_OPERATOR'].nil? || ENV['OTEL_RUBY_OPERATOR'] == 'true' ? '/otel-auto-instrumentation-ruby' : nil | ||
additional_gem_path = operator_gem_path || ENV['OTEL_RUBY_ADDITIONAL_GEM_PATH'] || Gem.dir | ||
puts "Loading the additional gem path from #{additional_gem_path}" if ENV['OTEL_RUBY_AUTO_INSTRUMENTATION_DEBUG'] == 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment regarding diagnostic messages. Consider using $stderr
for these so they can be siphoned by users that rely on structured logs coming from $stdout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use $stdout.puts
|
||
source 'https://rubygems.org' | ||
|
||
# avoid use gemspec to avoid circular load |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused about this. Aren't these already declared in the gemspec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified the Gemfile
end | ||
|
||
# this is used for case when user particularly want to enable single instrumentation | ||
OTEL_INSTRUMENTATION_MAP = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do really like this and wished that the registry would support it. We only need a fully qualified name if the gem does not match the existing module pattern.
# OTelBundlerPatch | ||
module OTelBundlerPatch | ||
# ref: https://github.com/newrelic/newrelic-ruby-agent/blob/dev/lib/boot/strap.rb | ||
def require(*_groups) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def require(*_groups) | |
def require(...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Thank you for re-reviewing, @arielvalentin!
There's an open PR to change the naming to replace "zero-code" in favor of "automatic" on the public docs: open-telemetry/opentelemetry.io#7157 They're having some CI issues, so the changes haven't been deployed yet. |
# Copyright The OpenTelemetry Authors | ||
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think though that what we want is to see what "real world" applications would do. They require bundler and we are hooking into that flow. So event though we are requiring bundler as part of the Ruby run, wouldn't we want our examples to reflect how people would use it?
module OTelBundlerPatch | ||
# Nested module to handle OpenTelemetry initialization logic | ||
module OTelInitializer | ||
OTEL_INSTRUMENTATION_MAP = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be clear. This will add the constant to Bundler::Runtime as well right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will be part of Bundler::Runtime.
I also added another rails example for validation (with separated readme).
|
||
Bundler::Runtime.prepend(OTelBundlerPatch) | ||
|
||
Bundler.require if ENV['OTEL_RUBY_REQUIRE_BUNDLER'] == 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the relationshiop between using Bundler.require
and if someone uses bundler/setup
?
Will this work as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the user’s Ruby file looks like this?
require 'bundler/setup'
Bundler.require
If OTEL_RUBY_OPERATOR
is set to true
, the gem path from /otel-auto-instrumentation-ruby
will be added to $LOAD_PATH
.
Ideally, /otel-auto-instrumentation-ruby
should only contain OpenTelemetry-related gems. If it includes dependencies that the OpenTelemetry SDK relies on, and the user’s gem path contains the same gems but with different versions, version conflicts may occur. To avoid this, the user can define OTEL_RUBY_UNLOAD_LIBRARY
to exclude specific gems (e.g., google-protobuf
).
If OTEL_RUBY_OPERATOR
is set to false
, the default Gem.dir
will be loaded into $LOAD_PATH
.
require 'bundler/setup'
only adds the gems specified in the Gemfile to $LOAD_PATH
, so if everything is configured correctly, it should not cause conflicts.
Will this work as expected?
Do you mean: will the script still inject the auto-instrumentation code if the user has both bundler/setup
and Bundler.require
?
Yes—it should work as expected. In fact, a default Rails app includes both, and it worked in the demo we tested last time.
…trib into auto-instrumentation
Description
This PR aims to contribute a script/gem that can load the necessary OpenTelemetry-related gems and initialize the SDK for the opentelemetry-operator. The script/gem also includes a resource detector as part of its features.
The main idea was inspired by new_relic, which prepends the bundler.require function and places the loading script at the end of the loading procedure. For apps or scripts that don't use Bundler or a Gemfile, more details on variations of app loading can be found in the README. Currently, the goal is to ensure that Rails apps can work, with future improvements in mind.
The reason behind creating this gem is to facilitate easy installation and dependency enforcement. Although for the OpenTelemetry-Operator Node.js, it puts the loading script directly in the src folder (and then copy over to binding volume), it still relies on the auto-instrumentation-node package to install all required packages (e.g., API/SDK and instrumentation packages).
The main purpose of the gem is to serve the OpenTelemetry-Operator, which requires more configuration changes (see the configuration section in the README). Users don't need to modify their Ruby app codebase while using the instrumentation from OpenTelemetry-Ruby, but anyone can still use it in any environment.
The gem name, file structure, and implementation need more suggestions to make them more reasonable. I have tested it with rails and sinatra (through rackup), and I currently still in the process of testing against the opentelemetry operator cluster locally. Once the operator testing done, I will mark the pr as ready.