Skip to content
This repository has been archived by the owner on Nov 15, 2020. It is now read-only.

Adding slf4j-simple to resolve KPL output missing issue #38

Closed
wants to merge 1 commit into from

Conversation

Jackyjjc
Copy link

To resolve issue reported here: #36 for logstash 2.x

Basically the same change as 1fd2cc6. However, logstash 2.x does not have log4j in its classpath so I included slf4j-simple instead. The downside is that there is no obvious way to change the log level for KPL so the output will be a bit noisy.

@@ -32,6 +32,8 @@ Gem::Specification.new do |s|
s.add_runtime_dependency "logstash-codec-json", "< 3.0.0"
s.add_development_dependency "logstash-devutils"
s.add_development_dependency "gem-release", "~>0.7.3"
s.add_development_dependency "rake", "< 10.2.0"
s.add_development_dependency "kramdown", "< 1.15.0"
Copy link
Author

@Jackyjjc Jackyjjc May 23, 2018

Choose a reason for hiding this comment

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

On my local machine bundle couldn't resolve the version for these two libs without these version restriction. Bundle will try to pull the latest of these libs but the latest requires ruby 2.0. Could be my configuration issue?

@Jackyjjc
Copy link
Author

Jackyjjc commented May 23, 2018

I've patched this to resolve the issue an internal team has. They are using the patch version already but I feel like I should submit the patch back to upstream in case anyone else is affected by this.

If you think 2.x is at the end of life and don't want to support it anymore then it is fine :)

@asammut
Copy link

asammut commented May 24, 2018

@samcday would it be possible to please get this PR merged :-)

@asammut
Copy link

asammut commented May 31, 2018

Hi @samcday could we please get this PR merged ASAP. We are currently flying blind with our logstash implementation as many log lines are not making to the log files due to this.

@Jackyjjc
Copy link
Author

@asammut I thought we are rolling out with the fork?

@asammut
Copy link

asammut commented May 31, 2018

I was treating the fork more as a last resort. I don't quite understand why we wouldn't want to merge this and fix logging for everyone who uses this plugin

@samcday
Copy link
Owner

samcday commented May 31, 2018

@asammut not sure if you're new to OSS, but here's a free tip - being demanding to open source maintainers will probably make them want to actively not help you :)

Being an ex-Atlassian I know there's still pockets of people/teams that have an allergic reaction to the idea of using / maintaining forks. Given the history I can totally understand that. But, treating a fork as a "last resort" arbitrarily is somewhat silly, for a couple of reasons:

  1. As I pointed out in my first comment, logstash 2.x series is very much EOL. If you were using a supported version of Logstash, you'd be using the latest version of this plugin which fixes the issue already.
  2. If you're so gung ho about this change, then it means you're confident in it, which should mean you'd have no qualms about running it as a fork for some period of time ;)

The reason I didn't rush out and merge this is I'm a little sketched out about adding slf4-simple to the classpath like this. Anyone else using Logstash 2.x and other plugins that also use slf4j may have already added slf4j-log4j or one of the other adapters to their classpath. Upgrading to this version might then cause their logging to go screwy.

Instead of lobbing slf4j-simple into this plugin, can't you simply add it to your own Logstash installation lib dir? AFAICT if you drop JARs into $LSHOME/vendor/jruby/lib they'll be added to the main Logstash classpath on boot-up.

@asammut
Copy link

asammut commented May 31, 2018

@samcday sorry, we didn't want to add the overhead of maintaining a forked repo for this simple change.

I agree it's not ideal that we are not using the latest supported version of logstash but this is where my team is at. We aren't in a position to upgrade at the moment and that's not going to change any time soon.

In regards to the $LSHOME/vendor/jruby/lib suggestion, I have actually tried that solution and it didn't work for us.

@samcday
Copy link
Owner

samcday commented Nov 15, 2020

Hi there, I'm archiving this repo (#51). I'm sorry for whatever protracted length of time I ignored your issue/PR for!

@samcday samcday closed this Nov 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants