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

[WIP] Event object accessors and field reference API #5035

Closed
colinsurprenant opened this issue Apr 6, 2016 · 8 comments
Closed

[WIP] Event object accessors and field reference API #5035

colinsurprenant opened this issue Apr 6, 2016 · 8 comments

Comments

@colinsurprenant
Copy link
Contributor

The new Java Event implementation changes the way plugins or Ruby filter scripts can interact with the Event object, in particular for setting fields values in nested objects. Some implementation specific assumptions cannot be made anymore as we move forward with new implementation strategies and eventually offer new alternate languages APIs.

This brings forward the need to clearly define the Event object API from a plugin or Ruby filter script developer. I believe this API was never formally defined and the 5.0 release cycle would be a good time to correct this.

To help with the evolution of the implementation, we should avoid relying on implementation and language specific idioms and provide a clear API and guidelines to our core objects from a plugin development context.

In that context and precisely for the Event accessors and field reference case, we should adopt the field reference syntax as the only way to manipulate Event inner values and we should document that the returned values when looking up a field reference are copies of the Event values and modifying these values is not guaranteed to automatically update the Event. An updated value must by explicitly written back into the event using the field reference setter syntax.

For example, imagine an Event which has this nested structure:

event["[foo][bar]"] ==> "baz" 

Moving forward the following construct to update the value in the [foo][bar] path would not be supported:

event["foo"]["bar"] = "zab"

Note in the example above, it is assumed that event["foo"] returns a reference to a Ruby hash that contains the bar key which can be in-place updated.

Likewise with this same assumption but written a bit differently:

h = event["foo"]
h["bar"] = "zab"

The proper way is to explicitly set an updated value using the complete field reference like this:

event["[foo][bar]"] = "zab"

Or alternatively:

h = event["foo"]
h["bar"] = "zab"
event["foo"] = h

This relates to #5028 #4834 #4999

@guyboertje
Copy link
Contributor

We need to bear this in mind #4743

@colinsurprenant
Copy link
Contributor Author

@guyboertje can you please provide a tl;dr here for what we should bear in mind in #4743 ?

@guyboertje
Copy link
Contributor

OK
If a reference points to a scalar. e.g. event["[foo][bar]"] --> "baz" and then one updates a descendent path event["[foo][bar][qux]"] = 42, we get Java::JavaLang::ClassCastException: expecting List or Map. "baz" is not a Map.
We can mitigate for this by promoting "bar" => "baz" into the Map that replaces the "baz" value
e.g. event["[foo][bar]"] --> {qux=42, _bar=baz} but we need to agree on the appropriate mitigation.

This can be made to happen in the mutate filter and add_field in any plugin.

#4743 - Discussed various mitigation approaches.

@guyboertje
Copy link
Contributor

In the same vein, should we forbid/allow the replacement of a Map with a scalar?
e.g. if we have this event["[foo][bar]"] --> {qux=42, _bar=baz} then should this be allowed event["[foo][bar]"] = 42?

@colinsurprenant
Copy link
Contributor Author

@guyboertje these are very valid concerns and should be properly defined in the API. maybe we can start with the legacy behaviour and see what makes sense moving forward?

@colinsurprenant
Copy link
Contributor Author

@guyboertje I am copying your accessors path invalidation bug sequence from #5212 :

> event = LogStash::Event.new
=> #<LogStash::Event:0x25ce435 @metadata_accessors=#<LogStash::Util::Accessors:0x7ea71fc2 @store={}, @lut={}>, @cancelled=false, @data={"@version"=>"1", "@timestamp"=>"2016-04-15T14:19:31.561Z"}, @metadata={}, @accessors=#<LogStash::Util::Accessors:0x7cd5fcf4 @store={"@version"=>"1", "@timestamp"=>"2016-04-15T14:19:31.561Z"}, @lut={}>>
> event["[a][0]"]
=> nil
> event["[a][0]"] = 42
=> 42
> event["[a]"]
=> {"0"=>42}
> event["[a]"] = [42,24]
=> [42, 24]
> event["[a]"]
=> [42, 24]
> event["[a][0]"]
=> 42
> event["[a]"] = [24,42]
=> [24, 42]
> event["[a][0]"]
=> 42
> event["[a][0]"] = {"a"=> 99, "b" => 98}
=> {"a"=>99, "b"=>98}
> event["[a][0]"]
=> {"a"=>99, "b"=>98}
> event["[a]"]
=> [24, 42]
> event["[a][0]"]
=> {"a"=>99, "b"=>98}
> event["[a][0][b]"]
TypeError: can't convert String into Integer
  from org/jruby/RubyFixnum.java:1143:in `[]'

I wrote a spec with this sequence and confirm the problem on both the Ruby and Java Event implementations.

@colinsurprenant
Copy link
Contributor Author

colinsurprenant commented Apr 15, 2016

Actually ^^ I will move that into a separate issue - #5127

@colinsurprenant
Copy link
Contributor Author

This ruby hash-like Event API will be dropped in 5.0, see #5140 - closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants