Skip to content
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

feat: Version Compatibility Based on Gem Specs #1025

Conversation

arielvalentin
Copy link
Contributor

@arielvalentin arielvalentin commented Nov 20, 2021

This changes provides instrumentation authors the ability to use gem specifications for version compatibility checks instead of having to write a custom compatibility block.

In order to take advantage of this feature instrumentation authors must:

  1. Declare the library and version constraints as a development dependency
  2. Use instrumentation naming conventions e.g. opentelemetry-instrumentation-ruby_kafka

Related open-telemetry/opentelemetry-ruby-contrib#373

Some Concerns:

  • There are certainly many more test scenarios to take into consideration. E.g. When multiple versions of the gem are installed and not yet activated.
  • Naming is strict and requires that the instrumentation name suffix matches the target library. We currently have some cases where the instrumentation names do not match e.g. ruby-kafka and activesupport

@arielvalentin arielvalentin marked this pull request as draft November 21, 2021 02:48
@arielvalentin arielvalentin marked this pull request as ready for review November 21, 2021 03:45
arielvalentin and others added 13 commits November 22, 2021 19:07
This changes provides instrumentation authors the ability to use gem
specifications for version compatability checks instead of having to
write a custom compatability block.

In order to take advantage of this feature instrumentation authors must:

1. Declare the library and version constraints as a development dependency
2. Use instrumentation naming conventions e.g. `opentelemetry-instrumentation-ruby_kafka`

Related https://github.com/open-telemetry/opentelemetry-ruby/issues/988
@arielvalentin arielvalentin force-pushed the arielvalentin/gemspec-dependencies branch from 1f80352 to 82ca716 Compare November 23, 2021 01:08
@arielvalentin
Copy link
Contributor Author

Ping @open-telemetry/ruby-maintainers @open-telemetry/ruby-approvers 🙏

@ericmustin
Copy link
Contributor

@arielvalentin My two cents is i think this looks great and is an improvement on the current system. It feels _ a little_ hacky to me to rely on development dependancies in a gemspec, for something like an instrumentation that doesn't necessarily need to be packaged as a gem to begin with, but since this doesn't override the compatible block, but only acts as a useful fallback/default, I don't see the harm. that being said I'd like some other folks to chime in here, I brought this up in the SIG, would be keen to hear other POVs here.

@arielvalentin
Copy link
Contributor Author

@arielvalentin My two cents is i think this looks great and is an improvement on the current system. It feels _ a little_ hacky to me to rely on development dependancies in a gemspec, for something like an instrumentation that doesn't necessarily need to be packaged as a gem to begin with, but since this doesn't override the compatible block, but only acts as a useful fallback/default, I don't see the harm. that being said I'd like some other folks to chime in here, I brought this up in the SIG, would be keen to hear other POVs here.

@ericmustin I would think folks that are writing their own auto-instrumentation that are not packaged as a gem are likely either implemented first party or patched in their applications. It's likely they are aware what the compatibility requirements are in their own projects or may not be using instrumentation at all.

Could you elaborate a bit on what you think makes it a little hacky?

@ericmustin
Copy link
Contributor

Could you elaborate a bit on what you think makes it a little hacky?

I don't think that checking development dependancies in a gemspec from production code is a common convention. Maybe hacky isn't the right term. That being said I think it's fine imo, just, unusual. Again i'd like some other input here from @open-telemetry/ruby-maintainers @open-telemetry/ruby-approvers

@arielvalentin arielvalentin force-pushed the arielvalentin/gemspec-dependencies branch from ea1c911 to 45e53d9 Compare November 30, 2021 06:48
@arielvalentin
Copy link
Contributor Author

@robertlaurin do you have any time this week to take a look?

@arielvalentin
Copy link
Contributor Author

Closing per my offline conversation with @robertlaurin open-telemetry/opentelemetry-ruby-contrib#373

@arielvalentin arielvalentin deleted the arielvalentin/gemspec-dependencies branch January 21, 2022 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants