Skip to content
This repository has been archived by the owner on Jun 29, 2023. It is now read-only.

Kafkasender #173

Closed
wants to merge 1 commit into from
Closed

Kafkasender #173

wants to merge 1 commit into from

Conversation

rifatdover
Copy link
Contributor

@rifatdover rifatdover commented Dec 24, 2018

This PR add functionality to logstash-gelf to log Kafka as Producer

Make sure that:

  • You have read the contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

@rifatdover
Copy link
Contributor Author

Please do not merge. I think there is a problem similar to danielwegener/logback-kafka-appender#44

@rifatdover
Copy link
Contributor Author

rifatdover commented Dec 27, 2018

Well the problem emerges with SL4J. KafkaProducer request a Logger with package name org.apache.kafka or javax.management while it is sending. And creates a deadlock.

Also it is possible working with different appenders for them:

 <root level="ALL">
        <appender-ref ref="gelfKafka" />
    </root>
    <logger name="org.apache.kafka" level="ALL" additivity="false">
        <appender-ref ref="gelfUdp" />
    </logger>
    <logger name="javax.management" level="ALL" additivity="false">
        <appender-ref ref="gelfUdp" />
    </logger>

Well even if I don't like that it will be ok for everyone. Because what I believe Kafka would be used for a failover scenerio of Graylog down in an application architecture. And when graylog is down nobody will care about these logs?

Adding it in documentation limitations section would do the job. What do you think? @mp911de

Copy link
Owner

@mp911de mp911de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your pull request. I added a few review comments. I see two major issues here:

  1. We shouldn't add another config system to configure Kafka but rather stick to how we configure e.g. the Redis sender and move all extended properties into the URL. We also should not extend appender config/properties for technology-specific extensions as this requires changes to our public API and causes confusion where to configure settings.
  2. Logger deadlock: The mentioned limitation/workaround is inevitable if we integrate with third-party dependencies that require a logger – we're already in a logger component and so (re)using a logger can easily lead to problems, that's why we introduced ErrorReporter. For Kafka, the only thing we can do is providing proper documentation and a warning about the issue so people do not discover this shortcoming in production.

Care to have a look and address the review comments?

README.md Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
@mp911de mp911de added the type: enhancement A general enhancement label Dec 27, 2018
rifatdover pushed a commit to rifatdover/logstash-gelf that referenced this pull request Jan 2, 2019
@mp911de
Copy link
Owner

mp911de commented Jan 18, 2019

Awesome work! Thanks a lot. Can you do the two following things to make merging a bit easier:

  • Squash your commits to a single commit
  • Reformat the newly introduced classes using our Eclipse formatter settings. Some files contain double-linebreaks and this makes it easier to have consistent formatting.

@mp911de mp911de added this to the 1.13.0 milestone Jan 18, 2019
@rifatdover
Copy link
Contributor Author

All done 👍

mp911de pushed a commit that referenced this pull request Jan 25, 2019
Log events can now be shipped using Kafka. Appenders can configure a Kafka URL in the host field according to the scheme:

kafka://broker[:port]?[producer_properties]#[log-topic]

e.g. kafka://localhost#topic-log

Kafka uses internally logging and JMX components with logging so these logging categories should be excluded to prevent circular, recursive log calls.

Original pull request: #173.
mp911de added a commit that referenced this pull request Jan 25, 2019
Remove Kafka transport documentation from readme as transports are documented on the site. Add timeouts to Kafka send futures. Simplify tests. Remove log4j test as log4j 1.x is out of maintenance.

Make fields final where possible. Add author tags, fix javadoc tags.

Original pull request: #173
@mp911de
Copy link
Owner

mp911de commented Jan 25, 2019

Thanks a lot. That's merged and polished now.

@mp911de mp911de closed this Jan 25, 2019
@rifatdover rifatdover deleted the kafkasender branch January 28, 2019 17:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants