-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added plugin.name to fish tag log lines #11078
Conversation
97cca26
to
195b8b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far - can we add an integration test to show the plugin name logging works?
Ok I do it!
…On Wed, Sep 4, 2019 at 3:29 PM Rob Bavey ***@***.***> wrote:
***@***.**** commented on this pull request.
Looks good so far - can we add an integration test to show the plugin name
logging works?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#11078?email_source=notifications&email_token=AAH5RUII4CCPZPVCX3EJAJTQH6Z4JA5CNFSM4IPA4P2KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDTZ55A#pullrequestreview-283614964>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAH5RUJJQ7SXSXUVQEMQRPDQH6Z4JANCNFSM4IPA4P2A>
.
|
195b8b0
to
baa5300
Compare
@robbavey this is done and ready to review |
46451b1
to
bc11ab8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the work done here in terms of how the extra information is added to log entries, but it might make sense to hold off on committing this PR until we have a more useful plugin.id
and use that rather than include plugin.name
.
My thoughts here are that plugin.name
on its own may not be massively useful - the name does not unique identify plugins of the same type in a pipeline (or whether the plugin is an input, output, filter or codec), and often the log category already gives us sufficient context to do identify which plugin generated the log message (although this is less the case for more Java-centric plugins):
eg - this log entry created using a Logstash with this PR included:
[2019-09-20T10:42:05,574][INFO ][logstash.outputs.elasticsearch][main][elasticsearch] Retrying individual bulk actions that failed or were rejected by the previous bulk request. {:count=>2}
Thoughts @jsvd?
} | ||
IO.write(@ls.application_settings_file, settings.to_yaml) | ||
@ls.spawn_logstash("-w", "1" , "-e", config) | ||
#@ls.wait_for_logstash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Please remove comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f4c638f
to
3b83b21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nitpicks, but other than that, looks good!
@@ -10,6 +10,7 @@ | |||
import org.jruby.internal.runtime.methods.DynamicMethod; | |||
import org.jruby.runtime.Block; | |||
import org.jruby.runtime.builtin.IRubyObject; | |||
import org.jruby.runtime.ThreadContext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: unused import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -11,6 +11,7 @@ | |||
import org.jruby.runtime.Block; | |||
import org.jruby.runtime.ThreadContext; | |||
import org.jruby.runtime.builtin.IRubyObject; | |||
import org.logstash.RubyUtil; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Unused import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
bddcb96
to
1d3c539
Compare
Added the plugin's name to log lines generated by the plugins
This PR adds
plugin.id
to log lines.Tipical use case:
plugin.id
bring that id (running Java pipeline)curl -XGET "localhost:9600/_node/pipelines/main?graph=true"
)How to test
bin/logstash -f "<config>" --java-execution=true --debug
logs/logstash-plain.log
you should find a line like: