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

Support for JRuby #148

Merged
merged 25 commits into from
Aug 21, 2019
Merged

Support for JRuby #148

merged 25 commits into from
Aug 21, 2019

Conversation

Adithya-copart
Copy link
Contributor

These are the minor changes that I had to add to get instana working on JRuby.

Ideally, it'll be easier for us to use instana if the changes in this PR are made on the gem directly as that would eliminate the need for maintaining a forked version and also enable other JRuby users to use this gem.

The GC metrics are not available directly in the ruby process dashboard but instana agent taps onto the JVM and displays GC metrics on a separate dashboard for us.

  • Added conditional loading of gems that require C-extensions in JRuby.
    I added another file that is responsible for loading OJ when the gem is available, so that MRI
    users need not take any performance hits.

  • Bumped the version of sys-proctable to resolve JRuby+Mac issue: Undefined method read_uint64 in JRuby. djberg96/sys-proctable#77. We could also consider loading sys-proctable conditionally in OSX if I understand Drop sys-proctable usage #108 correctly.

  • Updated the logic of checking the host OS in agent.rb.

  • Added socket.closed? guard in the ensure block to get rid of the socket related errors while trying to call socket.close on a already closed socket.

@pglombardo Thanks for the help and pointing in the right direction.

@Adithya-copart Adithya-copart mentioned this pull request Aug 12, 2019
@Adithya-copart
Copy link
Contributor Author

I did not change travis config to run the tests on JRuby.

However, I manually verified it in my workstation. It’ll be nice if JRuby is included in the travis pipeline.

@pglombardo
Copy link
Contributor

This is great work @Adithya-copart! I'm glad you got something useable for JRuby now.

Unfortunately, we can't merge this into the mainline until we decide to officially support JRuby - meaning we put effort into on-going support, development, updates, bug fixes, documentation etc..

Also note that tracing with this setup, you are likely to receive broken or incomplete traces between the JRuby and Java parts since they are not aware of each other.

We have the feature request filed in our backlog but as of yet, there hasn't been any concrete plans to pursue this.

With that said, I may merge some of these changes via cherry-pick. I'll update as things develop.

@Adithya-copart
Copy link
Contributor Author

Also note that tracing with this setup, you are likely to receive broken or incomplete traces between the JRuby and Java parts since they are not aware of each other.

Thanks for the insight and the response, we don't use a lot of Java code directly but we do use via the JDBC database adapter etc.

We will make sure to keep that in mind.

@pglombardo
Copy link
Contributor

I spoke to @CodingFabian today and actually the tracing point should be non-issue as JRuby won't be touched by the Java instrumentation currently. We'll also aim to merge this PR sooner than later so at least we will work under JRuby for the time being until we add official support. 😎

lib/instana/agent.rb Outdated Show resolved Hide resolved
Use substring search to verify the host OS.
@pglombardo pglombardo merged commit 7a92315 into instana:master Aug 21, 2019
@pglombardo
Copy link
Contributor

This has been released in gem 1.10.1 - thanks again @Adithya-copart!

https://github.com/instana/ruby-sensor/releases/tag/1.10.1

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.

None yet

2 participants