SPIKE: Gem Version Compatibility Checks

[arielvalentin]

This PR is not intended to be merged. It is a spike to flesh out alternative implementations of instrumentation version compatibility.

[arielvalentin]

@robertlaurin @ericmustin This is a first step in a multi stage refactoring to add some constraints around what versions an auto-instrumented library will support.

Before I proceed I would like to get your initial thoughts and feedback.

cc: #947

• As a first step I added instance template methods and moved the evaluation logic into the Base class. I left the compatible block in place as a way to override the default behavior. The old logic assumed if no compatibility block was provided it would try anyway. Is that correct?
• The next step here would be to implement the template methods in the instrumentations who currently do not declare min versions.
• The last step would be to convert the template methods to instance variables and inject them using declarative class methods (A.K.A. DSLs)

The only headache here for me is the redundant VERSION constant as I am going to need it for our situation where RubyGem specifications are not available to us via loaded specs. This makes me wonder if I am going about this the wrong way and if we should rely on only the VERSION constant being present.

The other thing bouncing around in my head is reducing test friction. Would there be a way to validate the these constraints without combinatorial explosion of appraisals?

Also, how does the instrumentation semver relate to the target library?

[ericmustin]

broadly speaking i think what you've laid out here would work, but practically speaking i'm not sure it actually accomplishes too much beside making things more confusing for the benefit of a little bit of automagic on the compatibility check, especially since your use case of not wanting to rely on loaded_specs still isn't met. Additionally, it doesn't really help solve the idea of a max_version like discussed here open-telemetry/opentelemetry-ruby-contrib#373.

Here's what I think we've gotten to

1. We want to be able to define a min and max version in some way that is clean and easy to understand
2. we want to have confidence about what versions an instrumentation supports
3. we want to be able to allow users to have a fallback that doesn't require loaded_specs[<gem_name>]

I think we could do this a few ways but the most straightforward way i could think of for now is just to

1. painstakingly go thru each instrumentation and update the compatibility check so it checks a min version against both loaded_specs and a fallback of <whatever constant exists in that gem that defines version, which is usually 'VERSION' but not always>. If we are feeling like we have bandwdith, we could also check against a max version in situations where we feel it's appropriate. we could set the max version as a constant MAX_VERSION. with those constants set we could perhaps more easily generate appraisals.

   • note: Yes, this approach would increase the number of test combinations significantly, but my 2 cents is I'd rather us have confidence in our tests and those tests take awhile to run, rather than not have a well tested gem. I think part of delivering a production quality observability gem is ensuring it's safe to use and attempting to reduce observer effect as much as reasonable, and if that means "our test suite takes 2 hours to complete" well, this is the business we've chosen

2. if we start introducing max versions, we may also want to give users the flexibility to say "i know what i'm doing, i dont need your stupid compatibility check" and let them toggle off the compatibility check either on a per instrumentation, or on a service wide basis. This way we could be pretty defensive about max supported versions, since we can always tell users "hey just toggle off the compatibility check if you are confident that this instrumentation works on the next major version, even though we haven't found time to explicitly test this compatibility yet"

kinda thinking out loud...wdyt?

[arielvalentin]

broadly speaking i think what you've laid out here would work, but practically speaking i'm not sure it actually accomplishes too much beside making things more confusing for the benefit of a little bit of automagic on the compatibility check, especially since your use case of not wanting to rely on loaded_specs still isn't met. Additionally, it doesn't really help solve the idea of a max_version like discussed here open-telemetry/opentelemetry-ruby-contrib#373.

Here's what I think we've gotten to

1. We want to be able to define a min and max version in some way that is clean and easy to understand
2. we want to have confidence about what versions an instrumentation supports
3. we want to be able to allow users to have a fallback that doesn't require loaded_specs[<gem_name>]

I think we could do this a few ways but the most straightforward way i could think of for now is just to

1. painstakingly go thru each instrumentation and update the compatibility check so it checks a min version against both loaded_specs and a fallback of <whatever constant exists in that gem that defines version, which is usually 'VERSION' but not always>. If we are feeling like we have bandwdith, we could also check against a max version in situations where we feel it's appropriate. we could set the max version as a constant MAX_VERSION. with those constants set we could perhaps more easily generate appraisals.

To confirm you are advocating that each auto-instrumentation define a MIN_VERSION and MAX_VERSION constant by convention and enforce pessimistic versioning. Is that right?

E.g.

module OpenTelemetry
  module Instrumentation
    module Sinatra
      class Instrumentation  < OpenTelemetry::Instrumentation::Base
          MIN_VERSION = "2.0.7"
          MAX_VERSION = "2.9"

          compatible do
             spec_version = Gem.loaded_specs["sinatra"]&.version || Gem::Version.new(::Sinatra::VERSION)
             Gem::Version.new(MIN_VERSION) <= spec_version && spec_version >= Gem::Version.new(MAX_VERSION) 
          end
      end
    end
   end
end

That makes it possible to generate appraisals easier because we will be able to find the constants by convention. Is that right?

• note: Yes, this approach would increase the number of test combinations significantly, but my 2 cents is I'd rather us have confidence in our tests and those tests take awhile to run, rather than not have a well tested gem. I think part of delivering a production quality observability gem is ensuring it's safe to use and attempting to reduce observer effect as much as reasonable, and if that means "our test suite takes 2 hours to complete" well, this is the business we've chosen

👍🏼 This may be a matter of changing how our test suite runs to parallelize execution in a more interesting and meaningful way.

2. if we start introducing max versions, we may also want to give users the flexibility to say "i know what i'm doing, i dont need your stupid compatibility check" and let them toggle off the compatibility check either on a per instrumentation, or on a service wide basis. This way we could be pretty defensive about max supported versions, since we can always tell users "hey just toggle off the compatibility check if you are confident that this instrumentation works on the next major version, even though we haven't found time to explicitly test this compatibility yet"

kinda thinking out loud...wdyt?

🤔 the advantage here is that we could test out compatibility with newer versions without requiring code changes right?

module OpenTelemetry
  module Instrumentation
    module Sinatra
      class Instrumentation  < OpenTelemetry::Instrumentation::Base
          MIN_VERSION = "2.0.7"
          MAX_VERSION = "2.9"

          compatible do
             spec_version = Gem.loaded_specs["sinatra"]&.version || Gem::Version.new(::Sinatra::VERSION)
             Gem::Version.new(MIN_VERSION) <= spec_version && (config[:enforce_max_version] && spec_version >= Gem::Version.new(MAX_VERSION)) 
          end
      end
    end
   end
end

There is something that still isn't sitting right with me and I cannot quite articulate what is wrong. Maybe I need to write down all of the references to versioning to help me formulate some thoughts...

ℹ️ We define a minimal supported gem version as a development dependency for development and testing:

https://github.com/open-telemetry/opentelemetry-ruby/blob/main/instrumentation/mysql2/opentelemetry-instrumentation-mysql2.gemspec#L34

We add it as a development dependency to make it easier to include all dependencies without impacting the target applications runtime dependencies.

ℹ️ We sometimes define an appraisal for all gem versions we believe this gem supports

https://github.com/open-telemetry/opentelemetry-ruby/blob/main/instrumentation/mysql2/Appraisals

ℹ️ We always exercise a test suite for the appraised versions on all of our supported platforms

https://github.com/open-telemetry/opentelemetry-ruby/blob/main/.github/workflows/ci.yml#L15

ℹ️ We sometimes programmatically enforce compatibility constraints

https://github.com/open-telemetry/opentelemetry-ruby/blob/main/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/instrumentation.rb#L24

ℹ️ We sometimes fallback to constant if the gemspecs were not loaded

https://github.com/open-telemetry/opentelemetry-ruby/blob/main/instrumentation/ruby_kafka/lib/opentelemetry/instrumentation/ruby_kafka/instrumentation.rb#L31

I wonder if there is something that we can do to declare the dependent versions in a single way and leverage them in all of the scenarios laid out above?

Is it possibly to do that using the declared library dependency version defined in the gemspec of the current instrumentation?

[arielvalentin]

@ericmustin After doing a little research it looks like using Gem::Specification.find_by_name along with Gem::Requirement#satisfied_by?

irb(main):008:0> instrumentation_gemspec = Gem::Specification.find_by_name("opentelemetry-instrumentation-faraday")
=> Gem::Specification.new do |s|
...

irb(main):009:0> supported_gemspec = instrumentation_gemspec.development_dependencies.find { |spec| spec.name == "faraday" }

=> Gem::Dependency.new("faraday", Gem::Requirement.new(["~> 0.17.0"]), :development)

irb(main):010:0> requirement = supported_gemspec.requirement
=> Gem::Requirement.new(["~> 0.17.0"])

irb(main):019:0> installed_gemspec = Gem::Specification.find_by_name("faraday")
=> Gem::Specification.new do |s|
...

irb(main):021:0> installed_gemspec.version
=> Gem::Version.new("0.17.4")

irb(main):020:0> requirement.satisfied_by?(installed_gemspec.version)
=> true

irb(main):029:0> requirement.satisfied_by?(Gem::Version.new("0.16.9"))
=> false
irb(main):030:0> requirement.satisfied_by?(Gem::Version.new("0.18.0"))
=> false

I think what this will do is give us the ability to declare explicitly target gem requirements without having to create a separate declaration in the instrumentation source.

[ericmustin]

@arielvalentin I think I agree with all the ℹ️'s you've laid out. And looking at your example of Gem::Specification.find_by_name along with Gem::Requirement#satisfied_by?, It looks interesting but I'm not sure what you mean by

declare explicitly target gem requirements without having to create a separate declaration in the instrumentation source.

What would this look like exactly? are you saying we could possibly just determine the min/max requirements from the instrumentation's .gemspec , and then use that to determine compatibility?

I guess what's confusing to me is we're trying to do a few things at once here. Is there one of the issues that we've talked about that is most important to your use case or that you're blocked on? Or are you just interested in broadly improving/adding a DSL here? If it's the latter that's fine I just want to make sure i'm on the same page.

[arielvalentin]

@arielvalentin I think I agree with all the ℹ️'s you've laid out. And looking at your example of Gem::Specification.find_by_name along with Gem::Requirement#satisfied_by?, It looks interesting but I'm not sure what you mean by

declare explicitly target gem requirements without having to create a separate declaration in the instrumentation source.

What would this look like exactly? are you saying we could possibly just determine the min/max requirements from the instrumentation's .gemspec , and then use that to determine compatibility?

I guess what's confusing to me is we're trying to do a few things at once here. Is there one of the issues that we've talked about that is most important to your use case or that you're blocked on? Or are you just interested in broadly improving/adding a DSL here? If it's the latter that's fine I just want to make sure i'm on the same page.

It sounds like I wasn't clear about what I was suggesting in my previous comment.

I am abandoning the idea of the versioning DSL as well as falling back on a constant in favor of comparing the instrumentation gemspec and the target library gemspec as the source for the compatibility check at runtime. This should reduce friction for engineers since they can be as specific as they need to in gemspec dependencies instead of defining specific constraints in code.

Using Gem::Specification works in my use case where the loaded_gemspecs is not populated.

The only thing we would need to developer to specific is the name of the target gem they are instrumenting. The Instrumentation::Base class can take it from there.

I will update the PR with an example of what I mean tomorrow.

[ericmustin]

@arielvalentin ah gotcha gotcha, yea I think that makes sense, we should prob still accept an optional compatibility block in the case that the logic is more complicated (maybe, based on platform or some other arbitrary thing that we cant plan for)

[arielvalentin]

@arielvalentin ah gotcha gotcha, yea I think that makes sense, we should prob still accept an optional compatibility block in the case that the logic is more complicated (maybe, based on platform or some other arbitrary thing that we cant plan for)

I agree ... Circle gets the square!

[arielvalentin]

@ericmustin I hacked together a spike to get some feedback from you. LMK if you think it looks good.

[ericmustin]

@arielvalentin i think this approach lgtm, off the top of my head my largest concerns are all addressed

• this is backward compatible + can be added to instrumentation gems iteratively.
• this plays nicely with compatible blocks. As far I can tell, this does, so all good.
• the auto-magic of using gemspec+instrumented gem name is robust and just works

I will ask for some feedback from the other folks on my team but my 2 cents is I really like this approach and i think it improves instrumentation compatibility definitions (by baking it into the gemspec), and also creates a path to more tightly define+test our gem compatibility in appraisals (since we can abstract out the major support versions for appraisals programmatically from the gemspec).

Maybe can split this work into

• a PR for the instrumentation base class + 1 instrumentation + base tests,
• then we can follow up with a sweep of the other instrumentation.

Wdyt?

[arielvalentin]

Maybe can split this work into

• a PR for the instrumentation base class + 1 instrumentation + base tests,
• then we can follow up with a sweep of the other instrumentation.

Wdyt?

@ericmustin Yes that sounds good to me. I am on vacation for Thanksgiving week so let's allow this to sit for a bit to give others a chance at providing some feedback.

[arielvalentin]

Closing this out in favor of #1025