-
-
Notifications
You must be signed in to change notification settings - Fork 485
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
JRuby support #19
Comments
JSON is required as that is what the server speaks (as is the case with 99% of APIs these days). As for multi_json, I've got nothing against a patch like that, as long as on normal Ruby it continues to install a faster library by default. I'm not super familiar with JRuby, so I'm not sure if you can make conditional dependencies somehow. |
@coderanger I made the commit but since we don't have specs covering this piece of raven-ruby, I will run my fork on my staging environment to check if it's ok. See https://github.com/sobrinho/raven-ruby/commit/f80196498cfdaa82e79f02340b3365a7b034fb9d |
This will mean that if the user is on MRI and they don't manually install yajl or oj, they are stuck with the slower standard library one. I'm not a fan of that outcome. |
@coderanger you should think about how many times and how many important is the time spent on JSON encoding time. If it took 3sec to encode, could be ok if the application raises one exception per day. If the application raises 1,000 exceptions per minute, could be a problem. I think the mostly part of application raises few exceptions instead of a lot of exceptions. |
Sentry can also be used for straight logging, not just exceptions. |
Using multi_json make the developer decide which json implementation is better for each application. Some applications needs a fast json implementation but some don't, we can't guess that. Logging things to sentry does not mean the spent time on json encoding is expensive, it just means you are logging things. In highly accessed applications could be a problem, in smaller applications could not. If you need a fast json implementation, you just need to add it to gemfile and multi_json will take care about the rest: gem 'sentry-raven'
gem 'yajl-ruby' Using multi_json means we are not assuming things about application environment. If you find it necessary, we could write a read me section telling about multi json :) |
You make a compelling case, but the other side of the coin is good defaults. It just seems like the current state of Ruby packaging requires conservative defaults, rather than fast ones. |
@coderanger thanks :) see #20 ;) |
Just out of curiosity, was the JSON library the only thing that was breaking in JRuby? |
@lsb in that time, yeah. Did you find another issues? |
yajl-ruby does not compile in jruby.
On a mixed environment like ruby, python, java, etc I think XML works better.
If JSON is the best choice for raven-ruby, we could use something like multi_json and let developer decide which json library he want.
What do you think?
I can make a pull request if you accept the change to multi_json :)
The text was updated successfully, but these errors were encountered: