Update ElasticAPM::SpanHelpers to use ruby2_keywords for argument delegation

[danarnold]

What does this pull request do?

This code updates ElasticAPM::SpanHelpers to use ruby2_keywords to properly delegate method arguments in a way that's compatible with all supported versions of ruby.

The included test will raise an ArgumentError in ruby >=3 without these changes.

I've also added the ruby2_keywords gem to the Gemfile for ruby versions <2.7, as the method is not included in those older versions of ruby. The gem is already pulled in by multiple other gems that depend on it, so this isn't effectively adding anything, it's just making it explicit since this project now explicitly depends on it.

Why is it important?

ElasticAPM::SpanHelpers was previously broken with ruby >=3.0 because it uses the legacy format of delegating arguments. In ruby 3, if a method has a mixture of keyword arguments and positional arguments, any methods spanned would raise an ArgumentError. See here and here for more details. Switching to the new argument delegation format in ruby 3, which would be (*args, **kwargs, &block) instead of (*args, &block), would also fix the issue, but that wouldn't be backwards compatible with ruby versions before 3.0.

Checklist

• I have signed the Contributor License Agreement.
• My code follows the style guidelines of this project (See .rubocop.yml)
• I have rebased my changes on top of the latest main branch

• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

• I have made corresponding changes to the documentation
• I have updated CHANGELOG.asciidoc
• I have updated supported-technologies.asciidoc
• Added an API method or config option? Document in which version this will be introduced

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-09-26T14:30:02.345+0000

• Duration: 33 min 23 sec

Test stats 🧪

Test	Results
Failed	0
Passed	41927
Skipped	75
Total	42002

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run benchmark tests : Run the benchmark test.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[jaggederest]

Wow, this looks like a great contribution, thank you for the PR! I will make sure CI passes (just the docs left) and review it more thoroughly and then it'll be merged in straight away.

[jaggederest]

run elasticsearch-ci/docs

[jaggederest]

/test

[jaggederest]

Hi folks, can you rebase this on the main branch? I would happily do it for you but you're pulling from your own repo 👍 That should fix the tests, which I will rerun once the rebase is complete. Thank you again for the contribution

[danarnold]

Sure. I just rebased our branch onto the current elastic:main branch.

[estolfo]

Hi @danarnold, I'm not so familiar with this gem. You're saying that it's included as a core gem for ruby versions 2.7 and higher?
The only gems that the elastic-apm gem depends on are concurrent-ruby and http. So in the rare case that someone doesn't have ruby2_keywords installed via some other gem and they're on ruby version < 2.7, wouldn't this be a breaking change? Please let me know if I'm mistaken.

[danarnold]

You're saying that it's included as a core gem for ruby versions 2.7 and higher?

Yep, it shouldn't be necessary to specify the gem for versions 2.7 and higher. The ruby post I linked above states this.

The only gems that the elastic-apm gem depends on are concurrent-ruby and http. So in the rare case that someone doesn't have ruby2_keywords installed via some other gem and they're on ruby version < 2.7, wouldn't this be a breaking change? Please let me know if I'm mistaken.

You're right, I included ruby2_keywords in the Gemfile instead of the gemspec. I just pushed an updated commit adding ruby2_keywords to the gemspec so we can ensure it's present.

[estolfo]

Hey @danarnold I'd like to avoid adding more gem dependencies, if possible.
Would it be possible to check the ruby version and write the method as you've proposed versus the way it was depending on the whether the ruby version is 2.7? Also, we support JRuby, so it also not necessary to specify this gem for later JRuby versions?

[danarnold]

@estolfo I had already searched for a way to limit the dependencies in the gemspec to a specific version of ruby, but I didn't find any, unfortunately.

I understand being hesitant to add a new gem dependency. You may find it comforting to see just how minimal of a dependency this gem is: its purpose is really just to enable underlying support inside the ruby language by defining blank wrapper methods (see the source code here). Additionally, the gem isn't from some third party that would have to be relied on for continuing support; this is a gem owned and published by the core ruby language team, and is therefore as solid of a dependency as you can get. As I've alluded to previously, this gem is extremely common as a dependency for many other gems. The compatibility changes introduced with ruby 2.7 and ruby 3 have widespread effects, and many projects have had to make similar changes to ensure that their code doesn't break.

The only option to avoid the gem dependency would be to include the code that defines the wrapper methods directly in this gem's source code. Then the code could be wrapped in a conditional which would prevent it from being defined when it isn't necessary. To be honest, though, I can't see how this would be the more elegant solution. It seems to me to be a messier solution than just including the ruby2_keywords gem as a dependency.

Regarding your JRuby question: I don't personally have any experience with JRuby. A quick look at the JRuby homepage shows me that the latest release of JRuby, 9.3.8.0, is targeted at compatibility with ruby 2.6.x. Therefore, I don't think these changes would be relevant to JRuby just yet, nor would I expect them to cause any issues with JRuby, since ruby2_keywords is a backwards compatible change for C ruby 2.6.x and previous. I can't speak to the JRuby development team's intentions regarding future versions of JRuby, but it seems that if their goal is compatibility with a given version of C ruby, then I don't see any reason to suspect that they would deviate from that path and create an incompatibility around the argument delegation changes introduced in ruby 2.7 and ruby 3. For current versions of JRuby, running the additional unit test I wrote under JRuby should make it pretty easy to check and confirm that the code is compatible.

[estolfo]

Hi @danarnold thank you for the detailed response and explanation.
I've just tested these changes in another PR and it passes. Would you mind rebasing and then rerunning the tests?
Thanks!

[danarnold]

@estolfo Sure thing. I've just done that. The tests look good on my end.

[apmmachine]

🌐 Coverage report

Name	Metrics % (covered/total)	Diff
Packages	100.0% (1/1)	💚
Files	99.237% (130/131)	👍
Classes	99.237% (130/131)	👍
Lines	59.003% (2615/4432)	👍 0.009
Conditionals	100.0% (0/0)	💚

[estolfo]

/test

[estolfo]

@elasticmachine, run elasticsearch-ci/docs

[estolfo]

Thanks, @danarnold !