logstash-plugins / logstash-filter-translate

Translate filter for Logstash
Apache License 2.0
21 stars 47 forks source link

changed merge to update #57

Closed xoh closed 6 years ago

xoh commented 6 years ago

Entries deleted from a dictionary were not deleted from the internal hash, because Hash#merge! was called. Changed this behaviour to really update the dictionary.

Fixes #24

karmi commented 6 years ago

Hi @xoh, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

jsvd commented 6 years ago

@xoh very interesting...we don't document either behaviour. There's no mention that a refresh just updates entries or that it replaces the dictionary as a whole.

I'm tempted to avoid breaking the current behaviour as folks may be relying on it. We could easily provide a setting that defaults to the current behaviour but allows for your need too:

config :refresh_behaviour, :validate => ["merge", "replace"], :default => "merge"

What do you think?

xoh commented 6 years ago

To me it seems odd, that an update can only add, but not remove. But, as you said, some folks may rely on the current functionality and I don't like to break stuff.

I'll add a config setting defaulting to current behaviour and update the PR.

xoh commented 6 years ago

How may I simulate a changing dict-file?

xoh commented 6 years ago

Asking differently; I would need to change the internal value of dictionary_path after register:

subject.register
subject.filter(first_event)
subject.dictionary_path = altered_dict
subject.filter(second_event)

Any idea how to implement this?

xoh commented 6 years ago

Old versions of ruby (at least up to the version used by CI with jruby-1.7.27) have poor accuracy of Time.now (just 3 decimal places vs. 9 on current versions). This lead to translate not updating the dictionary_file sometimes, but there is/was no actual issue with the filter-code, just with the test-cases. Fixed them, so everything should by ready to merge(?)

jsvd commented 6 years ago

Wow, many thanks @xoh for all the time ! I'm giving it a final look now

jsvd commented 6 years ago

@xoh I took the liberty to take your commit and change the specs a little bit to remove dependency on timing and the extra file.

I merged your commit to master (e33dc0a7c156d85ffde43174210dfaae1d5f19c5) and I'll publish 3.1.0 in a few minutes. Many thanks for your contribution, fast responses and patience.

jsvd commented 6 years ago

version 3.1.0 has been published