search

aws / aws-xray-sdk-ruby

The official AWS X-Ray Recorder SDK for Ruby
Apache License 2.0
60 stars 58 forks source link

JRuby Support #4

Open dgolombek opened 6 years ago

dgolombek commented 6 years ago

The oj gem is MRI specific. Will you accept a patch to convert this gem to use multi_json so that it picks a JRuby friendly JSON library when appropriate? It defaults to Oj, when that gem is available. This will also allow users who've standardized on some other JSON library to continue using it, without pulling in another JSON gem.

haotianw465 commented 6 years ago

Thank you for your feedback. As long as the patch doesn't break MRI it should be fine. However, the SDK is not tested under JRuby, especially a few places the SDK is not tested to be thread-safe under JRuby due to the lack of GIL.

You are welcome to provide PRs or feedback on JRuby support so we can prioritize our work accordingly.

dgolombek commented 6 years ago

Can you tell me about the ruby 2.3.6 version requirement? JRuby is 2.3.X compatible, but currently hardcodes RUBY_VERSION as 2.3.3.

I think I have everything else working now, and will be ready to push up once the version issue is dealt with.

haotianw465 commented 6 years ago

Ruby 2.3.6 has a few security fixes https://www.ruby-lang.org/en/news/2017/12/14/ruby-2-3-6-released/. We generally don't support versions that have known security vulnerabilities.

Do you have issue installing the SDK as a dependency because of this?

dgolombek commented 6 years ago

Unfortunately there's no way to have parts of the gemspec be conditional on ruby interpreter, so right now it's a hard blocker. I can ask the JRuby team about bumping that version number a tiny bit higher, but I suspect they just want to wait until JRuby 9.2.0.0 is released, with Ruby 2.5.X compatibility. So yes, for now it's a hard blocker for JRuby.

haotianw465 commented 6 years ago

I'm sorry to hear that. Our beta version SDK is tested under 2.3, 2.4 and 2.5 with only MRI. For other interperters we would like to hear some feedback from users to decide the priority to support them.

For JRuby based on the discussion I think there are three main issues:

  1. RUBY_VERSION not compatible with JRuby 9.1.x
  2. Oj is MRI specific
  3. The SDK is not tested when GIL is not present.

From the JRuby website "JRuby 9.1.x is our current major version of JRuby. It is expected to be compatible with Ruby 2.3.x and stay in sync with C Ruby." If C Ruby has security fixes I don't see a reason they want to still hard code an older minor version.

I will check with JRuby on what is the latest. But please feel free to update what you find with them as well. Then we can discuss the next step.

dgolombek commented 6 years ago

Ok, I checked with JRuby team and they suggested that JRuby 9.2 (with MRI 2.5 compatibility) is being worked on for a end of April release and doing the minor version bump was more effort than it was worth before then (since there may not be another release prior to 9.2).

I'll put together the rest of changes now and we'll get those in, then when JRuby 9.2 comes out, I'll do further testing on the thread-safety piece. One of the services I'll be incorporating this into has hundreds of threads active most of the time, so it'll get a workout.

dgolombek commented 6 years ago

I finally have a chance to get back and work on this some more, now that JRuby 9.2.0.0 has been released. However, something odd happened with the 0.10.2 release that was pushed to RubyGems -- it still requires the oj gem (despite having removed that from the gemspec). I can't reproduce this locally by building the gem, so I'm wondering if perhaps the gem was built in an unclean environment?

Separately, do you have particular places of concern for thread safety? Some of the lazily allocated attrs in Entity are problematic, as are the Metadata namespaces (depending upon how they're used). I haven't thought through all the usecases yet, but the TLS-based Contexts are a good start to limit threading issues.

haotianw465 commented 6 years ago

It looks like a miss on our end. The 0.10.2 release doesn't have oj related commit. Sorry for the confusion. We will roll it out soon.

This gem went through thread safety testing under MRI. It's usually the places where multiple threads adding/streaming subsegments with the same parent (which is owned by their parent thread) that might cause problems. To officially announce support for JRuby we will need to do thorough testing and potentially add locks in a few places.

JRuby support is on our backlog but we can't guarantee an ETA. You are welcome to submit a PR and we are happy to work with you to get this support out.