logstash-plugins / logstash-integration-kafka

Kafka Integration for Logstash, providing Input and Output Plugins
Apache License 2.0
32 stars 60 forks source link

Kafka Output support manipulating kafka message headers #162

Closed LaithSiriani closed 7 months ago

LaithSiriani commented 7 months ago

Thanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link: https://www.elastic.co/contributor-agreement/

LaithSiriani commented 7 months ago

Due to previous failure in the Travis CI build, I modified the commit and uploaded a fix for some flakiness in the "when KafkaProducer#send() raises a non-retriable exception" test case

LaithSiriani commented 7 months ago

@roaksoax Hi, would it be possible to review this pull request? What would be needed to include this feature?

LaithSiriani commented 7 months ago

@mashhurs Sorry for the possibly unnecessary involvement, I saw you had reviewed the last pull request. What is the way of working to get a code review for the pull request?

LaithSiriani commented 7 months ago

Hi @yaauie, @mashhurs , the comments have been addressed.

mashhurs commented 7 months ago

If we don't have an explicit use-case for making the keys variable, then I would suggest avoiding the sprintf on the keys (and changing the docs to reflect that only the values are interpolated).

Thank you so much @LaithSiriani for addressing all comments and moving forward. Just to clarify @yaauie's last comment:

If we don't have an explicit use-case for making the keys variable, then I would suggest avoiding the sprintf on the keys (and changing the docs to reflect that only the values are interpolated).

We would opt out sprintf operation for now if your case doesn't require it.

LaithSiriani commented 7 months ago

Hi @mashhurs , the comment was explicitly about the key, which has been addressed, the value does have a use case to be interpolated by the sprintf

mashhurs commented 7 months ago

New v11.4.0 version released with this change.