Use @metadata for ENV variables

[untergeek]

This is a breaking change. We should do some notification if we publish this gem.

[untergeek]

This is in response to elastic/logstash#3944 (comment)

[suyograo]

What if some users want fields loaded from environment variables? They need to explicitly copy it from @metadata, right? I am fine with that because it is typically not required to index, for example

[untergeek]

Yes, exactly. Explicit copy if they want it in the output. It seems it's much more likely to be used for filtering than for output, though.

[suyograo]

+1, LGTM

[untergeek]

And if included, it's also more likely to be used in sprintf format.

[suyograo]

@untergeek can you also update the docs to reflect this change? i.e lets make it more explicit how to use this on the top of the class. We can also stick in an example where an explicit copy can be used

[untergeek]

@suyograo you mean beyond what I already put in line 11?

https://github.com/untergeek/logstash-filter-environment/blob/feature/4/lib/logstash/filters/environment.rb#L11

[untergeek]

I may have pushed that commit right before you saw it.

[suyograo]

@untergeek I'd also like to have something described at the top of the class: https://github.com/untergeek/logstash-filter-environment/blob/feature/4/lib/logstash/filters/environment.rb#L6

Users typically read this description before they dive into config --

"This filter lets you access environment variables and sets it in metadata field, blah, blah. You can then use it in other parts of pipeline ... . To make this part of your event in your destination, you need to copy .."

[cstockton]

@untergeek Thanks for the rapid response, this is a great change and the first time I've seen the @metadata field, not sure how I missed it. The only suggestion / tweak I may have is to maybe have an additional configuration setting in the plugin to avoid the BC break if it may cause some pain for people.. i.e. add_metadata_from_environment.

As for the @metadata feedback.. Given the difference in meaning and the value it adds maybe it could justify it's own set of configuration settings similar to the add_field, *_field familly I.E. -> add_metadata, *_metadata .. you only save an @metadata declaration but you gain a certain bit of explicitness and value to the api nomenclature. Then plugins that it makes sense to put the values in the metadata field could provide getters/setters to do so without hindering backwards compatibility as companies upgrade their current deployments. Just my two cents.

Thanks again!

[untergeek]

Thanks for the feedback, @cstockton

We have the opportunity with the coming of Logstash 2.0 to make some breaking changes. While I agree about the effect it will have on users, and the need to change their configurations to make use of @metadata, I'd like to keep the configuration here as simple as possible.

Unless a plurality of the other Logstash team members want to add the complexity, I think we'll probably stick with the current, simple configuration as it stands. We'll add plenty of documentation and notification before we actually publish the updated gem, even if that comes before the 2.0 release.

[suyograo]

@untergeek thoughts about renaming :add_field_from_env to add_metadata_from_env to be more explicit? This would also partially address @cstockton concern by making this an explicit breaking change. Otherwise folks may have a hard time debugging why their env field (if they used it to populate a field in ES for example) is not getting set.

This change in config will make LS not start and provide a stronger safety net. Also it reflects the change in behavior. Thoughts?

[untergeek]

That makes good sense. I'll merge that in.

[suyograo]

add_metadata_from_env

[suyograo]

@untergeek minor comment in doc to update to add_metadata_from_env. Merge after that