kv should always overwrite the target field

[elastic]

kv should always overwrite the target field

[avleen]

This patch removes the check on if kv exists, it seems to fix this issues:

--- lib/logstash/filters/kv.rb     2014-04-29 21:37:18.335010728 +0000
+++ lib/logstash/filters/kv.rb  2014-04-29 21:37:20.230012427 +0000
@@ -182,23 +182,22 @@
     # Add default key-values for missing keys
     kv = @default_keys.merge(kv)

-    # If we have any keys, create/append the hash
-    if kv.length > 0
-      if @target.nil?
-        # Default is to write to the root of the event.
-        dest = event.to_hash
+    # If we have any keys, create/append the hash.
+    # If we don't, at least set @target as expected.
+    if @target.nil?
+      # Default is to write to the root of the event.
+      dest = event.to_hash
+    else
+      if !event[@target].is_a?(Hash)
+        @logger.debug("Overwriting existing target field", :target => @target)
+        dest = event[@target] = {}
       else
-        if !event[@target].is_a?(Hash)
-          @logger.debug("Overwriting existing target field", :target => @target)
-          dest = event[@target] = {}
-        else
-          dest = event[@target]
-        end
+        dest = event[@target]
       end
-
-      dest.merge!(kv)
-      filter_matched(event)
     end
+
+    dest.merge!(kv)
+    filter_matched(event)
   end # def filter

   private

[colinsurprenant]

Thanks for reporting this and you solution proposal. We will review this. Is it possible for you to submit a pull request with your changes? Also it would be great if you could add specs for this in spec/filters/kv.rb. Let us know if you need help with writing specs.

[jordansissel]

@avleen If I'm reading your report correctly, you're saying that if you have kv { source => "foo" target => "foo" } and an event { "foo": "bar" } (no kv to parse!) that the result should be { "foo": { } } ?

[jordansissel]

Example -

% bin/logstash -e 'filter { kv { source => "message" target => "message" } }'
hello=world
{
  "message"    => {
    "hello" => "world"
  },
  ...
}

fancy pants
{
  "message"    => "fancy pants",
  ...
}

In the 2nd case, no kv was matched and left the original source alone (as a string). This would cause Elasticsearch to be upset if it the field were mapped as an object, right?

I'm not sure what the best default behavior would be, here.

Options when @source == @target:

• Promote @target to be empty hash {} as proposed by @avleen (I think?). This would cause data loss because we obliterate the old value.
• Promote @target to be a hash of { "_original" => <original field value> } so it's still a hash but we don't lose data.

Thoughts?

[avleen]

That's correct. This comes down to object mappings and potential loss of
data.

@target should always become a hash. Either it ends up empty, or it has
some data. But it's always a hash. If it's empty, just drop it at the end
of the run.

I think the user should get a choice in whether @target is overwritten, or
if the original is preserved. Different people may expect different things.

On Fri Jan 23 2015 at 2:28:29 PM Jordan Sissel [email protected]
wrote:

Example -

% bin/logstash -e 'filter { kv { source => "message" target => "message" } }'
hello=world
{
"message" => {
"hello" => "world"
},
...
}

fancy pants
{
"message" => "fancy pants",
...
}

In the 2nd case, no kv was matched and left the original source alone (as
a string). This would cause Elasticsearch to be upset if it the field were
mapped as an object, right?

I'm not sure what the best default behavior would be, here.

Options when @source == @target:

• Promote @target to be empty hash {} as proposed by @avleen
  https://github.com/avleen (I think?). This would cause data loss
  because we obliterate the old value.
• Promote @target to be a hash of { "_original" => } so it's still a hash but we don't lose data.

Thoughts?

Reply to this email directly or view it on GitHub
#1315 (comment)
.

[jordansissel]

For Logstash 1.5.0, we've moved all plugins to individual repositories, so I have moved this issue to logstash-plugins/logstash-filter-kv#11. Let's continue the discussion there! :)