Add 'metadata' option to include @metadata field in output #24
Conversation
|
I'm kind of uncertain about this change. The intent of event metadata was for having things that only live within a single Logstash instance and would never be shipped externally. Shipping metadata externally breaks this expectation. Can you help me understand your use case? Beyond that, my questions about this would start with this one: Why do you need to ship |
|
For clarification on the rubydebug codec: rubydebug can include metadata because the intent of that codec is to output an event for debugging purposes, so in debugging an event, it can be useful to have the metadata. |
|
The use case is any non-human debugging or testing tool that doesn't find the rubydebug output very compelling to parse. Specifically, I want to be able to use Logstash Filter Verifier to verify that |
|
@magnusbaeck @jordansissel FWIW I really like this idea. I don't see how making it a non-default option would be problematic. |
|
If everyone's happy with this change I can rebase my branch and make it ready for merging. |
|
@magnusbaeck I'd like to get an LGTM from @jordansissel before proceeding. |
|
I agree with the usefulness in debugging, and the use case here makes sense if there's no other way to debug a filter configuration. On the other hand, I am trying to remove Given I want to remove the APIs mentioned, I try to think about what could be next for your verifier -- This verifier could be an output plugin, or a custom pipeline implementation, instead of an external tool, which would give you access to the Event object and its metadata. I'm mostly brainstorming here ... Thoughts? |
|
So.. Ehhh. I'm on the fence. I really want to_json_with_metadata to disappear, but I like your use case and want to support it. If we can make to_json_with_metadata (or to_hash_with_metadata) deprecated on 5.0, and figure out how to keep supporting this use case without those methods, does that sound like an OK plan to you, @magnusbaeck? If so, I am +1 on this PR. |
|
FWIW, I am a +1 for this particular use case because at some point, I'd love to use Logstash Filter Verifier in our test suite :) |
|
@suyograo I think we can achieve verification without outputting @metadata, but I want to keep @magnusbaeck's work alive until we come up with that solution <3 |
|
Okay, so it sounds like it'd be worthwhile for me to rebase the branch then. How about merging #25 first so that I can include that change in this PR's changelog update? |
|
Alright, so to_json_with_metadata is already gone in the master branch of elastic/logstash.git so this patch won't work after a rebase. Should that method be reinstated then (but marked as deprecated)? And will you do that so that we can move forward? |
|
@magnusbaeck sorry for the delays. We have Event#to_hash_with_metadata that you could call and take that object and serialize it to json. |
| @@ -28,12 +28,21 @@ class LogStash::Codecs::JSONLines < LogStash::Codecs::Base | |||
| # Change the delimiter that separates lines | |||
| config :delimiter, :validate => :string, :default => "\n" | |||
|
|
|||
| # If true, the event's metadata (the `@metadata` field) will be | |||
| # included when used as an output codec. | |||
There was a problem hiding this comment.
Can we mark this :deprecated ? My hope is that we do not attempt to present metadata as a something to transmit between Logstash nodes since the intent was to keep metadata for a given event only within a single logstash instance.
Marking it deprecated lets us more easily replace this with what may be a better solution in the future.
Just like the rubydebug codec, being able to include the @metadata field in the output of the json_lines codec can be useful, for example for debugging or test tools that prefer machine-readable output. The new option is immediately marked as deprecated to discourage its use and to make it easier to replace it with something better once we figure out what that would be. Since the Event class's constructor mutates the input hash I had to refactor some existing tests to send in a clone of the hash since it'll otherwise end up without a @metadata field.
|
I really would like to see this PR to make progress to be able to finally have access to the @metadata in Logstash Filter Verifier (see magnusbaeck/logstash-filter-verifier#5). |
Just like the rubydebug codec, being able to include the
@metadatafield in the output of the json_lines codec can be useful, for example for debugging or test tools that prefer machine-readable output. Fixes #23.I'm not sure exactly when the
.to_json_with_metadatamethod was added and whether we need to bump any gem dependencies so please have a look at that.Added a minor version bump in the hopes that you'll be able to make a new release soon.