add Event#from_json method #4631

colinsurprenant · 2016-02-04T21:58:00Z

will fix #4595

This introduces Event#from_json which takes as argument a json string and returns a new Event object. The idea here is to update the json_lines codec to use this method to avoid double type conversion by first parsing the json to a Ruby structure then passing that structure to the Java Event and go through unnecessary type conversions. the POC has shown a ~50% performance increase using this.

Note that this PR adds specs for the input corner cases behaviour of the current deserialization behaviour to make sure from_json behaves the same way.

colinsurprenant · 2016-02-04T21:58:50Z

I will push a json_lines codec PR for this shortly to show the usage in a backward compatible way.

colinsurprenant · 2016-02-04T22:07:30Z

the build fail is because of the ruby-maven problem I believe, fixed in #4630. I will merge and rebase here to see.

andrewvc · 2016-02-04T22:09:08Z

@colinsurprenant isn't this a bit strange? Shouldn't we be just rewriting JSON Lines to be a java plugin?

andrewvc · 2016-02-04T22:13:36Z

Just to clarify, we've basically moved one codec into the Event class, isn't this not a pattern we want to encourage?

colinsurprenant · 2016-02-04T22:19:37Z

@andrewvc well, ultimately yes, having the codec in Java will be awesome, until then, this is the best option I believe. In any case, I think that if we have a to_json method, it does somehow make sense to also have a from_json? Since json ingestion support is such an important use-case to support, I think this will provide good value by bringing the performance improvement of today's Java Event implementation to to json ingestion use-case. WDYT?

colinsurprenant · 2016-02-04T22:21:14Z

rebased against master, let's see if it solves the ruby-maven bobo

colinsurprenant · 2016-02-04T22:24:21Z

@andrewvc also, this does not moves the codec into the event class, it moves the equivalent of LogStash::Json.load in Event
The codec still does the line tokenization etc

colinsurprenant · 2016-02-04T22:26:51Z

now the error is

rspec ./logstash-core/spec/logstash/runner_spec.rb:36 # LogStash::Runner argument parsing with no arguments should show help

which seems unrelated to these commits 😕

colinsurprenant · 2016-02-04T22:48:46Z

Ok, I am able to reproduce the build failure when on Java 7 and running

$ SPEC_OPTS="--seed 61716" rake test:core

IMO it is not related to this PR. Will create a separate issue.

The complete error is

Failures:

  1) LogStash::Runner argument parsing with no arguments should show help
     Failure/Error: subject.run(args)
       (#<Cabin::Channel:0x15462876>).warn("You may be interested in the '--configtest' flag which you can use to validate\nlogstash's configuration before you choose to restart a running system.")
           expected: 1 time with any arguments
           received: 2 times with arguments: ("You may be interested in the '--configtest' flag which you can use to validate\nlogstash's configuration before you choose to restart a running system.")
     # ./logstash-core/lib/logstash/runner.rb:164:in `execute'
     # ./vendor/bundle/jruby/1.9/gems/clamp-0.6.5/lib/clamp/command.rb:67:in `run'
     # ./logstash-core/spec/logstash/runner_spec.rb:40:in `(root)'
     # ./vendor/bundle/jruby/1.9/gems/rspec-wait-0.0.8/lib/rspec/wait.rb:46:in `(root)'
     # ./rakelib/test.rake:42:in `(root)'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/task.rb:240:in `execute'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/task.rb:235:in `execute'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/task.rb:179:in `invoke_with_call_chain'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/task.rb:172:in `invoke_with_call_chain'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/task.rb:165:in `invoke'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:150:in `invoke_task'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:106:in `top_level'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:106:in `top_level'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:115:in `run_with_threads'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:100:in `top_level'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:78:in `run'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:176:in `standard_exception_handling'
     # ./vendor/bundle/jruby/1.9/gems/rake-10.4.2/lib/rake/application.rb:75:in `run'

Finished in 13.78 seconds (files took 3.14 seconds to load)
1202 examples, 1 failure

Failed examples:

rspec ./logstash-core/spec/logstash/runner_spec.rb:36 # LogStash::Runner argument parsing with no arguments should show help

Randomized with seed 61716

andrewvc · 2016-02-04T23:00:55Z

@colinsurprenant makes sense for now! So long as we don't add any more from/to methods I'm 👍

colinsurprenant · 2016-02-04T23:01:31Z

ok, cool. who's next? 😬

talevy · 2016-02-04T23:30:30Z

logstash-core-event-java/src/main/java/com/logstash/ext/JrubyEventExtLibrary.java

+            try {
+                return RubyString.newString(context.runtime, event.toJson());
+            } catch (Exception e) {
+                throw new RaiseException(context.runtime, context.runtime.getModule("LogStash").defineOrGetModuleUnder("Json").getClass("GeneratorError"), e.getMessage(), true);


this usage of defineOrGetModuleUnder is slightly confusing.

If you end up defining it, then wouldn't the proceeding getClass return null? since nothing is using it after that fact, I suppose it is fine

it is and AFAIK this is the only way to get a reference of an inner module. maybe this could be established earlier on in the Library load so that if this namespace does not exist it will crash on startup and not at exception time. thanks, good observation.

talevy · 2016-02-04T23:32:35Z

in general, what is our stance on testing for our java-core + ruby integration? It looks like you chose to not introduce java tests in favor of doing ruby testing, should we stick to doing both? or is that only important once more of core becomes java. or did I miss the java tests somewhere?

colinsurprenant · 2016-02-05T00:11:40Z

@tal good point. in this case I did not include specific java tests and probably I should. there is a potential redundancy issue but I think it's fair that some tests overlaps but both are required since they both define different interfaces.

talevy · 2016-02-05T00:14:46Z

I don't view it as redundant. Since we going to be using direct java more and more, I view it as future-proofing :)

colinsurprenant · 2016-02-05T00:42:09Z

@talevy fair enough, I agree.

colinsurprenant · 2016-02-05T02:08:43Z

@talevy I think I covered what we discussed, initializing the error classes at load time and added Java Event#fromJson tests.

suyograo · 2016-02-05T02:32:25Z

heh, tests pass now on Java 7 http://build-eu-00.elastic.co/job/Logstash_PR/2170/

tal · 2016-02-05T02:36:01Z

Hi

talevy · 2016-02-05T02:39:08Z

hi @tal, meet @talevy

suyograo · 2016-02-05T02:41:11Z

We need a @tal vs @talevy PR showdown

talevy · 2016-02-05T02:42:36Z

I'm fine with that, we can assign all my tickets to the real @tal.

colinsurprenant · 2016-02-05T03:21:13Z

lol - sorry to have pulled you in @tal and thanks for stopping by and say Hi 😬

colinsurprenant · 2016-02-05T03:55:30Z

the related PR for the json_lines codec is at logstash-plugins/logstash-codec-json_lines#19

guyboertje · 2016-02-05T13:28:51Z

@colinsurprenant - are you doing the json codec too?

guyboertje · 2016-02-05T13:37:39Z

@colinsurprenant - I am presuming that the Java tests are only done in your IDE or is there some rspec magic that runs the Java tests too?

colinsurprenant · 2016-02-05T13:43:22Z

@guyboertje gradle build includes running tests, alternatively gradle test will only compile and run tests. If you don't have gradle 2.8+ installed locally you can use the provided wrapper gradlew.

colinsurprenant · 2016-02-05T14:03:57Z

@guyboertje good point, I actually forgot about the plain json codec and there is a subtlety in there... it has 2 behaviours depending on the input json structure type:

Object/Hash : it generates an Event just like the json_lines codec, all good.
Array : it generates an array of Event, one for each json array element.

I am not sure on the best strategy for this latter case. I could easily change the contract of from_json to output Event[] and deal with the array case in there. I think that would be the only way to deal with that efficiently otherwise we are stuck at doing the deserialization before, in the codec, in Ruby, then checking the resulting object type.

Hm. I believe changing the contract to

public static Event[] fromJson(String json)

or

public static List<Event> fromJson(String json)

would be acceptable? the added benefit would be to also support the array case in json_lines?

Thoughts?

guyboertje · 2016-02-05T14:56:42Z

@colinsurprenant

gradle test will only compile and run tests.

The reason for my question about the Java tests is: "Can the reviewer rely on the Jenkins test green tick in this PR to know that ALL the tests have been run and pass?".

I guess at the moment the reviewer must assume that if you are committing the jar file - the Java tests must have passed.

Is this the best we can do or can we make Travis do the gradle test and bundle exec rspec?

guyboertje · 2016-02-05T15:12:24Z

@colinsurprenant - I saw the json codec with its two operations, that is way I asked. 😄

I vote we change the fromJson contract to List<Event> - the jruby wrapper would convert this to a RubyArray of RubyEvent. In the codecs we can expect an array of events even if it has just one element.

colinsurprenant · 2016-02-05T15:56:15Z

@guyboertje

yup, agree with List<Event>.
the build process actually calls gradlew jar which does not includes running tests. good catch. we should definitely figure what the best strategy is for having the gradle test task invoked somewhere in the build process.
«I saw the json codec with its two operations» 😬 I appreciate your politeness in gently bringing this to my attention, maybe this has to do with this very British trait ;) but I wouldn't have been offended if you just stated your observation right there in the first place. But that's fine, we'll get there either way :P

guyboertje · 2016-02-05T20:04:30Z

@colinsurprenant - you give me more credit than I'm due. I did not leap as far as thinking that what happens in the json codec will affect the Java Event fromJson contract design. I only meant that there is an optimisation opportunity therein.

But I was also thinking, what happens if the JSON is a well formed array of scalars - but not suited for LS Event generation?

colinsurprenant · 2016-02-05T22:11:46Z

@guyboertje in this case I think the best/only thing to do is to honour the legacy behaviour which will be to throw an exception but the new implementation will throw an exception sooner by making sure the json array only contains maps, as for the legacy implementation would crash later when trying to initialize a new Event with a scalar parameter value.

I just finished the refactor to handle the json array of map case and I am a bit unhappy because it causes non negligible overhead in creating collections and converting these to Ruby and then the codec has to iterate the result. I am trying to see how this could be optimized...

colinsurprenant · 2016-02-05T22:24:37Z

@guyboertje I think I'll play the dynamic game and have from_json Ruby interface either return a single Event or Array<Event> and it's up to the method consumer to either know the passed json is a map yielding a single Event object otherwise testing on the returned object type result.is_a?(Array) or force-normalizing into an array Array(result). I will try to see if I can improve by doing this at the cost of a weirder interface. Suggestions welcome 😛

colinsurprenant · 2016-02-08T04:16:28Z

I finally decided on a middle ground, Event#from_json will always return an Array which makes for a more coherent interface but the implementation is using Java arrays instead of collections and this yields acceptable performance.

Note that this last commit introduces a bit more changes because I decided to upgrade Jackson which was really old and it is using a new namespace in v2

guyboertje · 2016-02-08T15:33:05Z

👍 on returning an array.

colinsurprenant · 2016-02-08T19:46:59Z

@talevy ^^ added test for partial invalid json array

talevy · 2016-02-08T21:11:58Z

sweet. any other todos? things look good to me

colinsurprenant · 2016-02-08T22:05:21Z

no, all good on my side. wanna merge. 🙏

talevy · 2016-02-08T22:57:29Z

LGTM

guyboertje · 2016-02-09T16:29:09Z

logstash-core-event-java/src/main/java/com/logstash/Event.java

+        Object o = mapper.readValue(json, Object.class);
+        // we currently only support Map or Array json objects
+        if (o instanceof Map) {
+            result = new Event[]{ new Event((Map)o) };


guyboertje · 2016-02-09T16:41:52Z

LGTM

colinsurprenant · 2016-02-09T16:43:35Z

great! squashing and merging.

from_json POC add Event#from_json with corresponding specs pre-inititalize error class constants tests for Event#from_json support array of events in from_json, upgrade to latest jackson add test for partially invalid json array

colinsurprenant · 2016-02-09T17:53:17Z

merged into master and 2.x

colinsurprenant added enhancement performance improvements java event v2.3.0 labels Feb 4, 2016

suyograo assigned talevy Feb 4, 2016

colinsurprenant force-pushed the feature/from_json branch from 2ac5ac2 to 8b4ab3b Compare February 4, 2016 22:20

colinsurprenant mentioned this pull request Feb 4, 2016

master build failure on java 7 #4632

Closed

talevy reviewed Feb 4, 2016
View reviewed changes

colinsurprenant mentioned this pull request Feb 5, 2016

use Event#from_json if available logstash-plugins/logstash-codec-json_lines#19

Merged

guyboertje reviewed Feb 9, 2016
View reviewed changes

new Event#from_json method

ad6cb41

from_json POC add Event#from_json with corresponding specs pre-inititalize error class constants tests for Event#from_json support array of events in from_json, upgrade to latest jackson add test for partially invalid json array

colinsurprenant force-pushed the feature/from_json branch from 777306b to ad6cb41 Compare February 9, 2016 17:50

colinsurprenant merged commit ad6cb41 into elastic:master Feb 9, 2016

colinsurprenant mentioned this pull request Feb 9, 2016

introduce Event#from_json to avoid type conversions in Java Event #4595

Closed

add Event#from_json method #4631

add Event#from_json method #4631

Conversation

colinsurprenant commented Feb 4, 2016

colinsurprenant commented Feb 4, 2016

colinsurprenant commented Feb 4, 2016

andrewvc commented Feb 4, 2016

andrewvc commented Feb 4, 2016

colinsurprenant commented Feb 4, 2016

colinsurprenant commented Feb 4, 2016

colinsurprenant commented Feb 4, 2016

colinsurprenant commented Feb 4, 2016

colinsurprenant commented Feb 4, 2016

andrewvc commented Feb 4, 2016

colinsurprenant commented Feb 4, 2016

talevy Feb 4, 2016

Choose a reason for hiding this comment

colinsurprenant Feb 5, 2016

Choose a reason for hiding this comment

talevy commented Feb 4, 2016

colinsurprenant commented Feb 5, 2016

talevy commented Feb 5, 2016

colinsurprenant commented Feb 5, 2016

colinsurprenant commented Feb 5, 2016

suyograo commented Feb 5, 2016

tal commented Feb 5, 2016

talevy commented Feb 5, 2016

suyograo commented Feb 5, 2016

talevy commented Feb 5, 2016

colinsurprenant commented Feb 5, 2016

colinsurprenant commented Feb 5, 2016

guyboertje commented Feb 5, 2016

guyboertje commented Feb 5, 2016

colinsurprenant commented Feb 5, 2016

colinsurprenant commented Feb 5, 2016

guyboertje commented Feb 5, 2016

guyboertje commented Feb 5, 2016

colinsurprenant commented Feb 5, 2016

guyboertje commented Feb 5, 2016

colinsurprenant commented Feb 5, 2016

colinsurprenant commented Feb 5, 2016

colinsurprenant commented Feb 8, 2016

guyboertje commented Feb 8, 2016

colinsurprenant commented Feb 8, 2016

talevy commented Feb 8, 2016

colinsurprenant commented Feb 8, 2016

talevy commented Feb 8, 2016

guyboertje Feb 9, 2016

Choose a reason for hiding this comment

guyboertje commented Feb 9, 2016

colinsurprenant commented Feb 9, 2016

colinsurprenant commented Feb 9, 2016