New Event API proposal

[colinsurprenant]

As per #5140 we have decided to remove Ruby hash-like API and expose new getter and setter in 5.0.

This is a WIP proposal and is open for discussions. The identified undefined behaviours will probably be the most discussed. Lets keep in mind that this is only the new getter and setter API, other new API proposals should be in their own issue which #5140 will track.

Ruby Event API Proposal

• A field reference is a string using the field reference syntax which is either a bare field name string like "foo" or using the nested syntax "[foo]" or "[foo][bar]".

Setter

# @param fieldref [String] field reference string
# @param value [Object] the value to set in that field reference
# @return [Event] this event or self for chainable calls
event.set(fieldref, value)

• Values are either string, numeric or timestamp scalar values, for example:

  event.set("foo", "baz")
  event.set("[foo]", "zab")
  event.set("[foo][bar]", 1)
  event.set("[foo][bar]", 1.0)
  event.set("[@metadata][foo]", "baz")

• Values can be arrays or hashes or nested

  event.set("[foo][bar]", [1, 2, 3])
  event.set("[foo][bar]", {"a" => 1, "b" => 2})
  event.set("[foo][bar]", {"a" => 1, "b" => 2, "c" => [1, 2]})

• When setting hash collections, the hash keys will become fields accessible as fieldref

  event.set("[foo][bar]", {"a" => 1, "b" => 2, "c" => [1, 2]})
  
  event.get("[foo][bar][a]") # => 1
  event.get("[foo][bar][c]") # => [1, 2]

• Mutating a collections after setting it in the Event has an undefined behaviour

  h = {"a" => 1, "b" => 2, "c" => [1, 2]}
  event.set("[foo][bar]", h)
  
  h["c"] = [3, 4]
  event.get("[foo][bar][c]") # => ????? but most probably [1, 2]

  This behaviour is certainly up for discussion, the main idea is that if you want to set or update a value in the event you have to use the explicit setter it cannot be assumed that mutating an object that was set in the event will also update it in the event, like this:

  h = {"a" => 1, "b" => 2, "c" => [1, 2]}
  event.set("[foo][bar]", h)
  
  h["c"] = [3, 4]
  event.set("[foo][bar]", h)
  # or better
  event.set("[foo][bar][c]", [3, 4]) 

  We say it is undefined because it is implementation specific and could change at any time so any observed behaviour is not an API contract.

Getter

# @param fieldref [String] field reference string
# @return [Object] the value at this field reference or nil if none
event.get(fieldref)

• Returned values are either string, numeric or timestamp scalar values, for example:

  event.get("foo" ) # => "baz"
  event.get("[foo]") # => "zab"
  event.get("[foo][bar]") # => 1
  event.get("[foo][bar]") # => 1.0
  event.get("[@metadata][foo]") # => "baz"

• Returned values can be arrays or hashes or nested

  event.get("[foo][bar]") # =>  [1, 2, 3]
  event.get("[foo][bar]") # => {"a" => 1, "b" => 2}
  event.get("[foo][bar]") # =>  {"a" => 1, "b" => 2, "c" => [1, 2]}

• Mutating a collections after getting it from the Event has an undefined behaviour

  h = event.get("[foo][bar]") # =>  {"a" => 1, "b" => 2, "c" => [1, 2]}
  h["c"] = [3, 4]
  h = event.get("[foo][bar][c]) # => ????? but most probably [1, 2]

  One way to avoid an undefined behaviour here would be to always return deep clones of the value at fieldref. This has obviously a rather high cost and I am unsure if this is a cost worth paying?

[jordansissel]

h = event.get("[foo][bar]["c"]) # => ????? but most probably [1, 2]

Is this a typo? Should it be event.get("[foo][bar][c]") (not ["c"]) ?

[suyograo]

Can you add an example for setting metadata?

event.set("[@metadata][foo]", "baz")
event.get("[@metadata][foo]")

[jordansissel]

@suyograo did you mean event.set("[@metadata][bar]") ? I may be reading too deeply, but I'm not sure what @foo is ;P

[suyograo]

oh yeah! Edited :)

[colinsurprenant]

@jordansissel corrected! @suyograo added!

[colinsurprenant]

Note that the new API wouldn't prevent someone from trying to do something like:

event.get("[foo][bar]")["c"] = [3, 4]

similar to what was done

event["[foo][bar]"]["c"] = [3, 4]
# or
event["foo"]["bar"]["c"] = [3, 4]

I am hoping that by using the get("...") method it will be more obvious that no value will be set in the event.

Thoughts?

[guyboertje]

+1 to all the above. People do have the most brutal to_hash_with_metadata, mutate dance, from_hash - but this should be discouraged because it will invalidate the whole cache.

[ph]

I am +1 for this interface.

One way to avoid an undefined behaviour here would be to always return deep clones of the value at fieldref. This has obviously a rather high cost and I am unsure if this is a cost worth paying?

I know the high cost, maybe its worth getting some numbers at least to reference in this issue.

[colinsurprenant]

@guyboertje invalidate the whole cache? which cache? the Accessors @lut ?

[colinsurprenant]

@ph we could but personally I am not sure I see the value. I am trying to balance the forced-safety, at a cost, vs the practical aspect. Here force-safety is meant to prevent developers from making mistakes because of ignorance of the behaviour, and the practical aspect is that there are very little chances this will happen in the first place. The typical flow is to set values in the event and any intermediate objects are discarded after the plugin invocation on the event. There are be corner cases but these are corner cases.

Do we want to impose forced-safety to prevent potential corner cases but at the expense of a performance penalty for all usage?

I think I'd be ok with providing proper documentation and letting the developer choose if they need to clone or not?

[ph]

The performance, was probably for my curiosity. :)

I think I'd be ok with providing proper documentation and letting the developer choose if they need to clone or not?

Since we are explicitly removing [] and []=, I think we are clear that we are not doing a mimic of a hash. So lets not create copies.

[guyboertje]

@colinsurprenant - I'm suggesting that for an existing event if someone does h = event.to_hash_with_metadata(); #mutate h; event.from_hash(h) then the from_hash should wipe and re-populate the LUT and PathCache.

[colinsurprenant]

@guyboertje PathCache does not need to be cleared, it is related to the field references seen in the config, OTOH the @lut does have to be cleared for sure, just like we will do on every set now because of the stale reference/invalidation in #5132

[suyograo]

This has been implemented in #5170